[cmake] On a fresh configure, remove possibly outdated pcms.#22751
Draft
hageboeck wants to merge 3 commits into
Draft
[cmake] On a fresh configure, remove possibly outdated pcms.#22751hageboeck wants to merge 3 commits into
hageboeck wants to merge 3 commits into
Conversation
Test Results 23 files 23 suites 3d 14h 28m 34s ⏱️ For more details on these failures, see this check. Results for commit 17202cc. ♻️ This comment has been updated with latest results. |
hageboeck
force-pushed
the
cmake_clean_pcms
branch
from
8ae89a0 to
ef2c9a2
Compare
hageboeck
force-pushed
the
cmake_clean_pcms
branch
from
473bbf0 to
3edb5e6
Compare
When CMake is run without an existing cache, or with cmake --fresh, remove leftover pcms the build directory. This can fix a build with broken pcms that don't get regenerated without needing to clean the entire build.
Previously, the cmake configure step was skipped when a build cached existed. This increases the risk that a change in the underlying container or in ROOT's config options goes undetected, and might e.g. lead to broken pcms. (The external build options are hashed for the time being, so they are expected to wipe the build directory, but this can break any time the hashing is touched) Here, the logic is changed such that cmake configure always runs, and that pcms are wiped. This should fix the occasional problem with broken pcms, and ensures that the ROOT build reacts correctly to changes in the underlying container.
hageboeck
force-pushed
the
cmake_clean_pcms
branch
from
3edb5e6 to
17202cc
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
When CMake is run without an existing cache, or with
cmake --fresh, remove leftover pcms inlib/. This can fix a build with broken pcms that don't get regenerated without needing to clean the entire build.Furthermore, ensure that we always run the cmake configure step in the CI. Previously, that step would be skipped when there was an existing CMake cache. This can prevent ROOT from picking up new build overrides, or from reacting to changes in the build containers (changes may only arrive once the precompiled artifacts are updated on S3).
Now, a configure with
cmake --freshis run unconditionally. If headers and build dependencies don't change, only the modules and dictionaries get rebuilt. The tests start to run at about 7 minutes into the CI job.More details
The underlying problem is the fact that rootcling_stage1 builds some pcms as side effects (e.g. std.pcm), and CMake doesn't know about these. It therefore can't know if they are out of date, and it can't instruct the build-system generator to clean them when the clean target is invoked, or update them if they are out of date.
Moreover, changes in the toolchain or in the installed packages might not be detected, since the build folder might have been generated in a different container than the one it's running on.
Although wiping the pcms only cures the symptoms (we should at some point build a full dependency tree with all pcms), it could make the life of the developers a bit better until all pcms are correctly tracked by CMake.