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. |
Lukas-Zenick
changed the title
ivanaamit
added
util
|
@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.
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.
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.
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.
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.
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.
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 sconsFollowing 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