[CMake] Never add current binary dir to command in ROOT_ADD_TEST
#20616
+1
−4
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
Test Results 22 files 22 suites 3d 22h 45m 33s ⏱️ For more details on these failures, see this check. Results for commit a802b4b. |
Member
|
As discussed in person, only removing the path won't be enough. Would use a system-installed ROOT if present. We either need to retain the path, rewrite the tests to use |
guitargeek
force-pushed
the
prefix_current_path
branch
from
a802b4b to
910ffce
Compare
hageboeck
approved these changes
Dec 3, 2025
Member
hageboeck
left a comment
hageboeck
left a comment
There was a problem hiding this comment.
LGTM, but maybe you could rephrase the first commit message:
- There's a typo
assimum - And part of the explanation should be that target names already get expanded. The change therefore only concerns executables that are not targets, and here it makes sense to not prefix them with the current binary directory.
And maybe you could rebuild "clean" because I would like to see the debian tests pass, too.
If a test added with `ROOT_ADD_TEST` uses a command that can't be found on the system and that is not an absolute path, the `ROOT_ADD_TEST` macro will implicitly prefix `$CMAKE_CURRENT_BINARY_DIR/`, assuming that the executable is supposed to be found in the current binary dir. However, that is not always a reasonable fallback, and can be even wrong if the executable was supposed to be found in the `PATH` that is set in the `ENVIRONMENT` for that test. So I think it's better to remove that fallback, requiring the tests to be explicit about which executable should be called. Note that this doesn't apply to the case when the command is a CMake target name, because then it will already be expanded to an absolute path by CMake.
The `root` executable should not be used directly in the test command, because if there was no `root` CMake target, it might use the literal string "root" in the command, potentially resolving to a ROOT install on the system and not the ROOT from the build directory. It is safer to use a `TARGET_FILE` generator expression here.
guitargeek
force-pushed
the
prefix_current_path
branch
from
910ffce to
1c5372c
Compare
Contributor
Author
|
Thanks! The first commit message is now updated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
If a test added with
ROOT_ADD_TESTuses a command that can't be found on the system and that is not an absolute path, theROOT_ADD_TESTmacro will implicitly prefix$CMAKE_CURRENT_BINARY_DIR/, assimum that the executable is supposed to be found in the current binary dir.However, that is not always a reasonable fallback, and can be even wrong if the executable was supposed to be found in the
PATHthat is set in theENVIRONMENTfor that test.So I think it's better to remove that fallback, requiring the tests to be explicit about which executable should be called.