[DO NOT MERGE] Check if #23056 would have caught alma8+ninja issues with #21346 - #23057
Draft
guitargeek wants to merge 5 commits into
Draft
[DO NOT MERGE] Check if #23056 would have caught alma8+ninja issues with #21346#23057guitargeek wants to merge 5 commits into
alma8+ninja issues with #21346#23057guitargeek wants to merge 5 commits into
Conversation
Contributor
Author
|
Ah, I forgot that we need to use |
guitargeek
force-pushed
the
ci-null-build-check_TRY
branch
from
August 10, 2026 08:37
1d01aa1 to
bf26800
Compare
guitargeek
changed the title
alma8 issues with #21346alma8+ninja issues with #21346
Test Results 23 files 23 suites 4d 5h 31m 43s ⏱️ For more details on these failures, see this check. Results for commit bf26800. |
Building an already built tree twice in a row must be a no-op, apart from etc/gitinfo.txt which the gitinfotxt target rewrites on every build by design. When that is not the case, every incremental build pays for the targets that are needlessly redone plus everything depending on them. Add a null build check that runs on all platforms right after the build: it fingerprints every file in the build tree, builds again, and reports what was written. Doing it this way rather than parsing build tool output keeps it working across Ninja, Makefiles and MSBuild alike. The verdict is only acted on at the very end, after ctest, so that a spurious rebuild does not cost us the test results. 🤖 Done with the help of AI
Contributor
Author
|
Confirmed! The problems on Alma 8 are seen in the CI run. |
ROOT_STANDARD_LIBRARY_PACKAGE(... NO_SOURCES) provided the dummy source file that add_library() needs as the output of a custom command that touches it. All the packages of one directory share that output, and listing one custom command output in several independent targets that build in parallel is not supported; core/clingutils alone has 15 of them. Ninja and Make happen to get away with it, but with MSBuild the touch runs again on every build, so 14 of the 15 STL dictionaries were recompiled and relinked every time, along with everything depending on them. Write the file at configure time instead. Nothing touches it at build time anymore, so it cannot go stale under the targets that compile it. 🤖 Done with the help of AI
MSBuild re-executes these post-build events even when the build has nothing to do, and `cmake -E copy` rewrites its destination unconditionally, so every null build modified those files. Ninja and Makefiles are immune because there POST_BUILD steps only run when the target itself relinks. Use `cmake -E copy_if_different` so a no-op event leaves the destination untouched, and replace the data.h POST_BUILD hack with configure_file(... COPYONLY), the convention the sibling test directories already use for such runtime inputs. 🤖 Done with the help of AI
Fixes root-project#21203. ROOT_GENERATE_DICTIONARY() relied on add_custom_command(IMPLICIT_DEPENDS) to rebuild a dictionary when one of its (transitively) included headers changed. IMPLICIT_DEPENDS is only honoured by the Unix Makefiles generator, is essentially broken even there, and cannot express cross-directory dependencies (see root-project#21203, root-project#20907). Incremental builds therefore routinely missed header changes. Replace it with a proper DEPFILE: * rootcling learns a "-MF <file>" option that writes a Makefile-format dependency file listing every header opened while generating the dictionary. * ROOT_GENERATE_DICTIONARY() passes "-MF" and hands the file to add_custom_command(DEPFILE ...), which is supported by the Ninja, Makefiles and IDE generators. Making the generated dependency file correct on every generator/platform (Ninja "deps = gcc", Unix Makefiles, Windows) required more than dumping the FileManager contents. The following points were needed and are the substance of the cross-platform fixes: * Target name. rootcling writes the dictionary to a temporary file and only renames it to its final name on success, so gOptDictionaryFileName already holds the temporary name ("G__X.cxx_tmp_<pid>") by the time the depfile is written. The original dictionary name is captured earlier and used as the depfile target instead. * Skip cling's in-memory buffers. The interpreter's FileManager also reports virtual buffers such as "input_line_N" and "<<< cling interactive line includer >>>". These are not real files: the latter contains spaces and angle brackets that corrupt the Makefile-format depfile (the parser splits it into bogus dependencies, which on Windows is compounded by the drive-letter colon), and none of them exist on disk, which would leave the dictionary permanently out of date. They are filtered out by requiring each entry to exist on disk, plus an explicit angle-bracket check. * Emit absolute paths. rootcling may be invoked from a different directory than the one the depfile is later consumed from. ROOT sets CMP0116 to OLD, so CMake does not rewrite the depfile, and a relative entry like "./Foo.hxx" (as produced for some roottest dictionaries) would be resolved against the wrong base and again leave the dictionary perpetually dirty. Every dependency is therefore made absolute. * Escape characters that are special to make (space, tab, '#', ':') and normalise separators to '/' so the file is valid on Windows too. The dependency file uses the ".depfile" extension rather than ".d" to avoid it being mistaken for a compiler-generated dependency file in another context. Validated with a full ROOT + roottest rebuild (Ninja): all dictionaries regenerate exactly once, after which the tree settles to a genuine no-op incremental build with no dictionary left perpetually dirty; touching a header regenerates precisely the dictionaries that include it. This resolves the roottest IO/tree/treeformula and Windows "*-build" failures seen on the draft PR.
guitargeek
force-pushed
the
ci-null-build-check_TRY
branch
from
August 11, 2026 07:58
bf26800 to
a06a9e9
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.
Check if #23056 would have caught
alma8+ninja issues with #21346