-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[CMake] Don't wrap command in ROOTTEST_COMPILE_MACRO
in CMake target
#19780
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Test Results 19 files 19 suites 3d 9h 1m 51s ⏱️ For more details on these failures, see this check. Results for commit c273e07. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrapping to command inside a Cmake target doesn't seem to be necessary, because in the test, the target is forced to be built without dependencies anyway (e.g.
cmake --build . --target my_macro_test/fast --always-make
when using makefiles).
This is only partially true, for Unix Makefiles
. With Ninja
, we don't use /fast
nor --always-make
AFAICT
The intention of the original macro is apparently to only build this one target that runs the command. The fact that this was not done correctly for Ninja doesn't mean that rebuilding all dependencies was the intended behavior. I suppose it was just not correctly implemented. |
94f401c
to
f215e1f
Compare
f215e1f
to
c4ce0be
Compare
In principle makes sense to me; maybe rephrase the first commit message because it still sounds wrong that dependencies would be ignored "anyway". Also adding @hageboeck because it's related to #19702 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the explanation confusing. I'll come discuss this in person.
Instead of running the `root ... <macro>.C+` build step as the command associated with a custom CMake target, just run the command directly in the test added with `add_test`. This simplification is possible because for the custom CMake target, no dependencies are ever specified. This makes building it not different from just running the underlying command directly.
* Remove preprocessor variables that were always defined and hence carried no information * Add include path and library path via environment variables * Remove redundant `gROOT->SetMacroPath()` call with path to source directory, because the `${realfp}` variable passed to the `build.C` script already contains the full path
c4ce0be
to
c273e07
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much clearer, thank you!
How does switching from an argument to an environment variable concretely help? If one ever tries to reproduce the failing compilation 'exactly', one now needs to copy paste (the extended version of):
instead of the extended version of:
|
That's true, the difference is marginal. But steering include and library paths via environment variables compared to C++ statements feels a bit cleaner to me, and also I think this was a good opportunity to test that the |
Instead of running the
root ... <macro>.C+
build step as the commandassociated with a custom CMake target, just run the command directly in
the test added with
add_test
.This simplification is possible because for the custom CMake target, no
dependencies are ever specified. This makes building it not different
from just running the underlying command directly.
Do some further CMake code simplifications in a second commit (more detail in the commit description).