-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[CMake] Simplify writing of modulemap. #19804
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
base: master
Are you sure you want to change the base?
Conversation
Test Results 20 files 20 suites 4d 3h 24m 23s ⏱️ For more details on these failures, see this check. Results for commit ec42560. ♻️ This comment has been updated with latest results. |
Instead of a two-step process that writes the dynamic contents at CMake configure time, and a build-time step that concatenates the static and dynamic part of the modulemap, CMake's configure_file() is used to directly write the full contents at configure time. As usual for configure files, the time stamp only changes when the file contents change, so spurious rebuilds of the modules are avoided.
COPYONLY) | ||
string(REPLACE ";" "" ROOT_GENERATED_MODULEMAP_CONTENT "${__modulemap_extra_content}") | ||
# Now append the generated content to the original modulemap | ||
configure_file(${CMAKE_SOURCE_DIR}/cmake/unix/module.modulemap ${CMAKE_BINARY_DIR}/include/ROOT.modulemap) |
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.
Would that work if somebody reconfigure with different options? Eg. cmake -DTMVA=On
then cmake -DTMVA=Off
.
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.
Yes, it does! I tested it, and the CMAKE variable is filled into the file again
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.
(and of course, when you reconfigure with different options, the time stamp is updated)
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.
Hm... okay. I tried that long time ago and failed to make it work but perhaps now we have all the dependencies tracked right...
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 concerns the build system, the only dependencies that remain are *.pcm -> ${BINDIR}/include/ROOT.modulemap
The contents of the modulemap are (re-)generated during every CMake run, and if it comes out to be the same contents, the file remains untouched.
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.
Btw there is this comment here
root/core/base/src/TSystem.cxx
Line 3572 in e928d42
// FIXME: Merge the modulemap generation from cmake and here in rootcling. |
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.
@vgvassilev is the comment above still relevant?
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.
Somewhat relevant but requires a lot of work. We could move all of the logic of modulemap creation to rootcling to merge the paths with Aclic. This however has other underwater rocks.
Instead of a two-step process that writes the dynamic contents at CMake configure time, and a build-time step that concatenates the static and dynamic part of the modulemap, CMake's configure_file is used to directly write the full contents at configure time.
This is carrying the simplifications in #19786 even further.