[CMake] Speed up copying of headers and tutorials #17607
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Move the copy of artifacts from build time to configure time, and use a faster copy function. Fix root-project#14953.
|
The error on Windows is related to this PR: |
Test Results 18 files 18 suites 4d 1h 53m 53s ⏱️ Results for commit 594345f. ♻️ This comment has been updated with latest results. |
d1d8c02 to
71f7aee
Compare
To copy headers to the build tree, ROOT was creating one target per header. Headers are copied in packs of 100 (Windows doesn't support very long command lines). This significantly reduces the number of CMake targets and speeds up the build. A simple test that only moves headers: make -j6 move_headers New Old 1st invocation 10.7 s 34.5 s 2nd invocation 2.7 s 3.8 s Fix root-project#14953.
b4d2aad to
594345f
Compare
@bellenot I might have found the problem: The command line seems to be too long. After splitting the copy command in packs of 100 (there is more than 1000 headers otherwise), the build seemed to succeed. Let's wait for the latest run. |
pcanal
reviewed
Feb 5, 2025
| DEPENDS ${CMAKE_SOURCE_DIR}/${artifact_file}) | ||
| list(APPEND artifact_files_builddir ${CMAKE_BINARY_DIR}/${artifact_file}) | ||
| endif() | ||
| file(COPY ${DIR_TO_COPY} DESTINATION ${CMAKE_BINARY_DIR} |
There was a problem hiding this comment.
The file(COPY_FILE command as a ONLY_IF_DIFFERENT options, does this syntax means that the file are copied at reconfigure time?
There was a problem hiding this comment.
I guess the question is whether they should be recopied every time. Currently that's the case, so one could move to COPY_FILE ... ONLY_IF_DIFFERENT.
However, this is not supported in CMake 3.20, which is our minimum CMake version.
pcanal
reviewed
Feb 5, 2025
| macro(ROOT_CREATE_HEADER_COPY_TARGETS) | ||
| get_property(HEADER_COPY_LISTS GLOBAL PROPERTY ROOT_HEADER_COPY_LISTS) | ||
| list(REMOVE_DUPLICATES HEADER_COPY_LISTS) | ||
| foreach(COPY_LIST ${HEADER_COPY_LISTS}) |
There was a problem hiding this comment.
Suggested change
| foreach(COPY_LIST ${HEADER_COPY_LISTS}) | |
| foreach(copy_list ${HEADER_COPY_LISTS}) |
We seems to tend to use lower case for variables ...
pcanal
requested changes
Feb 5, 2025
There was a problem hiding this comment.
With the copy of the artefact (seemingly) only at configure time, we have the following 'failures':
$ echo >> src/tutorials/hsimple.C
$ ls -l src/tutorials/hsimple.C tutorials/hsimple.C
-rw-r--r--. 1 pcanal us_cms 3572 Feb 5 17:41 src/tutorials/hsimple.C
-rw-r--r--. 1 pcanal us_cms 3571 Feb 5 16:23 tutorials/hsimple.C
$ ninja
[2/2] Updating etc/gitinfo.txt.
-- Found Git: /usr/bin/git (found version "2.43.5")
$ ls -l src/tutorials/hsimple.C tutorials/hsimple.C
-rw-r--r--. 1 pcanal us_cms 3572 Feb 5 17:41 src/tutorials/hsimple.C
-rw-r--r--. 1 pcanal us_cms 3571 Feb 5 16:23 tutorials/hsimple.C
I.e. the copied hsimple.C is not updated after a local modification in the source.
That was my intention. I wanted to reduce the number of CMake targets, since I consider the macros and tutorials mostly stable. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here are some results:
make -j6 move_headersmake -j6 move_artifactsAlthough there is one situation where the new approach takes longer (headers that end up in
<buildDir>/includewere touched), the benefits especially on first build or when no header was touched seem to outweigh the disadvantages.Fix #14953