-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
util, ext: Fix building TLM #1105
Conversation
Change-Id: If07fae2eb20ad62627e733573f61bc42d594f970
Hi @Lukas-Zenick, first of all thank you so much for the fix. I’d like to point out though the title of the issue you addressed is probably not correct. I believe the compilation error happens for non Arm simulations as well, it’s just that lots of the TLM doc has been written with Arm as an example. |
@ivanaamit Would it be possible to get these changes merged into develop by tomorrow? I have a project deadline and it's worth more for my grade if it gets merged in. If it still needs more intensive review then no worries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a expert at scons stuff but this lgtm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Lukas-Zenick we can merge it quickly but there's one actionable item on my side
util/tlm/SConstruct
Outdated
AddOption('--duplicate-sources', action='store_true', default=False, | ||
dest='duplicate_sources', | ||
help='Create symlinks to sources in the build directory') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need this one as the option is already present below (see --no-duplicate-sources). You just need to bump the option over here before it is used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "bump the option over here before it is used"? What I was referencing for this code was the main gem5 SConstruct file which has the following on line 145:
AddOption('--duplicate-sources', action='store_true', default=False,
dest='duplicate_sources',
help='Create symlinks to sources in the build directory')
AddOption('--no-duplicate-sources', action='store_false',
dest='duplicate_sources',
help='Do not create symlinks to sources in the build directory')
so I just assumed that you would always need to define both flags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to define both options. You just need to move the following code:
AddOption('--no-duplicate-sources', action='store_false',
dest='duplicate_sources',
help='Do not create symlinks to sources in the build directory')
To line 69.
@@ -44,6 +44,7 @@ env = Environment() | |||
|
|||
#Make the gem5 root available in SConscripts | |||
env['GEM5_ROOT'] = gem5_root | |||
env['GEM5BUILD'] = os.path.join(gem5_root, 'build') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a note: this won't work with kconfig based builds where the build directory is not necessarily in "build" and can have different names (see https://www.gem5.org/documentation/general_docs/kconfig_build_system/ ). Anyway I am ok on merging this for now as long as we document this issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, do I need to do anything specific in terms of documentation aside from having a code comment that specifies that it is not compatible with kconfig builds?
Change-Id: I36da18e8e8f6b587d66bfb33df069d297d0c74d2
Hi @giactra, could you please re-review this PR? I have applied the changes you requested. Let me know if you would like me to create an issue about the code not being compatible with Kconfig builds. Thanks. |
Thanks @ivanaamit . |
I am getting a "permission denied" message when trying to rebase. @giactra, if you could approve this PR, I will then do a "squash and merge" and change the commit message directly on GitHub. |
Fixed the issue that did not allow building TLM.
Build commands:
scons build/ARM/gem5.opt scons setconfig build/ARM USE_SYSTEMC=n scons --with-cxx-config --without-python --without-tcmalloc build/ARM/libgem5_opt.so cd util/tlm scons
Following this README, I tested it succeeds with the simple examples:
https://gem5.googlesource.com/public/gem5/+/master/util/tlm/README
GitHub Issue: #591
Change-Id: If07fae2eb20ad62627e733573f61bc42d594f970