[CMake] Use DEPFILE instead of IMPLICIT_DEPENDS for dictionaries - #23011
[CMake] Use DEPFILE instead of IMPLICIT_DEPENDS for dictionaries#23011guitargeek wants to merge 2 commits into
Conversation
Re-applying commit ae4d830 after revert in 94caecf. Closes root-project#21203.
Follows up on ae4d830, which switched ROOT_GENERATE_DICTIONARY() from IMPLICIT_DEPENDS to a proper DEPFILE, but rebuilt every dictionary on every incremental build with older Ninja (e.g. 1.8.2 on Alma 8): ```txt ninja: expected depfile '.../io/io/G__RIO.depfile' to mention 'io/io/G__RIO.cxx', got 'G__RIO.cxx' ``` rootcling writes the depfile target as the plain dictionary name it is given (`"G__RIO.cxx"`) relative to CMAKE_CURRENT_BINARY_DIR. ROOT pins `CMP0116` to OLD globally (see 7392b02), and under OLD the Ninja generator consumes the depfile verbatim and expects the target relative to the top-level build directory (`"io/io/G__RIO.cxx"`). The mismatch makes older Ninja treat the dictionary as perpetually out of date; newer Ninja tolerates it, which is why the regression only showed up on some platforms. This commit suggests to enable CMP0116 NEW locally around the dictionary `add_custom_command` so CMake transforms the depfile to whatever the active generator needs.
Test Results 23 files 23 suites 3d 15h 32m 36s ⏱️ Results for commit 3c63d03. |
| # use CMP0116 NEW locally so CMake normalises the depfile | ||
| # target for the active generator regardless of the global OLD setting. | ||
| cmake_policy(PUSH) | ||
| cmake_policy(SET CMP0116 NEW) |
There was a problem hiding this comment.
It seems that the current LLVM code can handle (when it needs to) CMP0116 to be NEW (See content of interpreter/llvm-project/llvm/cmake/modules/TableGen.cmake ; and the fact that the code refered to in 7392b02 in no longer in interpreter/llvm-project/cmake/Modules/CMakePolicy.cmake).
I would strongly recommend that we also (in a separate PR) set CMP0116 to be NEW globally.
| continue; | ||
| std::string filenameStr(absPath.str()); | ||
| // Skip the output dictionary file itself (final or temporary name). | ||
| if (filenameStr == dictOutputFileName || filenameStr == gOptDictionaryFileName.getValue()) |
There was a problem hiding this comment.
Are dictOutputFileName and gOptDictionaryFileName guaranteed to be absolute path name?
pcanal
left a comment
pcanal
left a comment
There was a problem hiding this comment.
Ideally this PR would verify by doing this (non trivial) flow in a fork (or equivalent tweaks no affecting the actual master):
- Merge (or another operation/hack that triggers the flow) so that that the build artefacts are produced and cached
- Create another no-op PR (i.e affecting only the README files) and verify that all the build have zero rebuilds.
|
On the surface this looks fine (but obviously needs through testing) |
I think there is an easier solution: checking in the CI that a second build is a no-op. I 'm exploring this idea in #23056 |
2 participants
Re-applying commit ae4d830 after revert in 94caecf.
This PR also contains a follow-up fix to make things work under CMP0116 OLD.
This version was confirmed to not result in spurious dictionary rebuilds on the
alma8Docker container.Closes #21203.