[CMake] Speed up copying of headers #17607
Merged
Conversation
This file contains hidden or 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
Member
|
The error on Windows is related to this PR: |
Test Results 18 files 18 suites 4d 16h 59m 42s ⏱️ Results for commit 11d1161. ♻️ This comment has been updated with latest results. |
hageboeck
force-pushed
the
header_copy
branch
2 times, most recently
from
b4d2aad to
594345f
Compare
Member
Author
@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
pcanal
reviewed
Feb 5, 2025
pcanal
requested changes
Feb 5, 2025
Member
pcanal
left a comment
pcanal
left a comment
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.
Member
Author
That was my intention. I wanted to reduce the number of CMake targets, since I consider the macros and tutorials mostly stable. |
hageboeck
force-pushed
the
header_copy
branch
2 times, most recently
from
feb75a7 to
cf683aa
Compare
Member
Author
|
@pcanal I decided to factor out the copy of artifacts into a second PR, so at least the improvement due to the faster copying of headers can be merged already. Could you have a look again? |
Member
Great. I was about to propose that :) |
pcanal
reviewed
Feb 10, 2025
pcanal
reviewed
Feb 10, 2025
hageboeck
changed the title
hageboeck
force-pushed
the
header_copy
branch
from
52f9418 to
1ff52c4
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. Co-authored-by: Philippe Canal <pcanal@fnal.gov>
The intention of __allBuiltins was to build builtins as part of
moving headers.
The existing code was seemingly creating a target dependency using
add_custom_target(move_headers DEPENDS ${__allBuiltins})
However, this argument only accepts file dependencies. The dependency
was therefore ignored. Since it doesn't seem to have caused any
problems, it is removed here explicitly.
hageboeck
force-pushed
the
header_copy
branch
from
d9f4b4e to
11d1161
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Move the copy of artifacts (e.g. tutorials) from build time to configure time, and use a faster copy function.Here are some results:
make -j6 move_headersFor the copy of artifacts, there is no agreement yet since it's more complicated. I will factor this part out into a second PR.
Fix #14953