[cmake] Do not auto force-enable or force-disable user CMake build options - #23092
[cmake] Do not auto force-enable or force-disable user CMake build options#23092ferdymercury wants to merge 10 commits into
Conversation
Test Results 23 files 23 suites 3d 19h 3m 27s ⏱️ For more details on these failures, see this check. Results for commit dbb1487. ♻️ This comment has been updated with latest results. |
…tions Messing up with the cache is dangerous, and users often complain about not having the control since the build system changes what they just passed as flag to the command line. Instead, convert these automatism into a helpful error message that the user can use as a hotfix to modify their build flags. This prevents surprises, annoying behind-the-scene changes, as well as bugs. Follows up on the phillosophy of root-project#23020 Fixes https://its.cern.ch/jira/browse/ROOT-10743
When cuda=On, but no viable compiler can be found, CMake produces the hard-to-understand error: Cannot determine link language of RooBatchCompute_CUDA. by hageboeck, cherrypicked from https://github.com/root-project/root/pull/23091/commits
e96b03f to
195baf7
Compare
|
@ferdymercury Can you add a comment to close the now seemingly superseded #18413 explaining where we actually ended up? |
fc53441 to
59dfd0d
Compare
|
Sure, comment added: #18413 (comment) |
…nd split multioptions and fix conflicting ssl=OFF vs builtin_openssl=ON on macos hotfix help message
Before, there was a subordinate hierarchical dependency, testing required testsupport. Now they have a logical OR relationship, GTest will be a dependency if one or the other is enabled.
as suggested by pcanal
23cb983 to
dbb1487
Compare
|
(asan failure seems unrelated) |
There was a problem hiding this comment.
Hello,
I think that's a great initiative, but I would approach this differently.
- First of all, I think we need to take a decision on what the "recommended" strategy is when a package is missing or a builtin can't be downloaded. Personally, I think that's up to the user: Install it, disable the component that requires it, or provide internet connection. Therefore, the hotfix suggestions (that are predominantly
-Dcomponent=Off) seem to mostly go in the wrong direction. - If you agree to the above, you will find that the
HOTFIX_BUILD_FLAGSlist will be mostly empty, so to shorten and simplify the code, I would remove it entirely. - If the hotfix list is removed, it would be consequent to use one single list or string to collect all failure messages in one place, and one list (already exists) to collect definitely missing pacakges.
That would bring us to a point where with any kind of problem, one could do the following:
if(<thereIsAProblem>)
string(APPEND SIS_FAILURE_MESSAGE "The x option contradicts y. Do <this> now.\\n")
endif()
# And at the very end of `SearchInstalledSoftware`:
if(DEFINED SIS_FAILURE_MESSAGE)
message(FATAL_ERROR ${SIS_FAILURE_MESSAGE})
endif()And if we are considering such a strategy, the ROOT_CHECK_CONNECTION macro can not only check the connection but also APPEND the relevant error message (see the inline comments).
Instead of adding 300 lines, this PR would probably even remove lines.
BTW: Not touching CACHE variables is great, thank you!
I didn't add comments to all instances that would need to be changed, but I hope it's clear what I have in mind.
| message(FATAL_ERROR "Option \"shared=Off\" not supported on Windows!") | ||
| list(APPEND HOTFIX_BUILD_FLAGS '-Dshared=ON') |
There was a problem hiding this comment.
These two contradict each other. Line 315 cannot be reached. It can simply be removed.
| ROOT_CHECK_CONNECTION("clad") | ||
| if(NO_CONNECTION) | ||
| message(SEND_ERROR "No internet connection, check it or disable the 'clad' option") | ||
| list(APPEND MISSING_PACKAGES 'clad') | ||
| list(APPEND HOTFIX_BUILD_FLAGS '-Dclad=OFF') | ||
| endif() |
There was a problem hiding this comment.
Replace all these lines with
ROOT_ENSURE_CONNECTION("clad")Clad is not really a missing package, in the sense that the user cannot easily install it.
| # Note: if the value of NO_CONNECTION is already FALSE, when calling the | ||
| # macro, the connection check will not run again. | ||
| #---------------------------------------------------------------------------- | ||
| macro(ROOT_CHECK_CONNECTION option) |
There was a problem hiding this comment.
This PR seems to add twice as many lines as it removes, couldn't we do it shorter?
I imagine going to macro(ROOT_ENSURE_CONNECTION <optionToBeDisabled>)?
In the macro, we detect the connection as before, but then:
# In the detect connection block that's only executed once:
message(WARNING "No internet connection. Builtin packages cannot be downloaded.")
# At the end of the macro:
if(NO_CONNECTION)
string(APPEND SIS_FAILURE_MESSAGE "The option -D<optionToBeDisabled> requires internet connection.\\n")
endif()There was a problem hiding this comment.
This PR seems to add twice as many lines as it removes, couldn't we do it shorter?
I imagine going to
macro(ROOT_ENSURE_CONNECTION <optionToBeDisabled>)?
Yes, definitely, I thought of adding macros, but I wanted to split this PR in two steps, first agreeing on what the behavior should be, then harmonizing and shortening with macros for whatever things are shared-common.
| if (x11) | ||
| message(SEND_ERROR "x11 (${x11_description}) and cocoa cannot be enabled simultaneously. Set -Dx11=OFF") | ||
| list(APPEND HOTFIX_BUILD_FLAGS '-Dx11=OFF') | ||
| endif() | ||
| else() | ||
| message(STATUS "Cocoa option can only be enabled on MacOSX platform") | ||
| set(cocoa OFF CACHE BOOL "Disabled because only available on MacOSX (${cocoa_description})" FORCE) | ||
| message(SEND_ERROR "Cocoa option can only be enabled on MacOSX platform. Set -Dcocoa=OFF") | ||
| list(APPEND HOTFIX_BUILD_FLAGS '-Dcocoa=OFF') |
There was a problem hiding this comment.
Since these are not really about optional (i.e. can use it or not) packages, I think message(FATAL_ERROR ...) makes sense. There's no other choice anyway, so stopping immediately seems like a good solution to me.
There was a problem hiding this comment.
Hmm, I agree, but wouldn't it make sense to split these changes into two distinct Pull Requests ?
First PR: we only change from CACHE to "print suggestion" but leave everything in the same direction
Second PR: we change behavior, ie direction of the automatism (ON or OFF) and nature of errors
In this case:
set(cocoa OFF CACHE with STATUS becomes:
please add -Dcocoa=OFF with SEND_ERROR
Adding a FATAL_ERROR instead is something I thought of avoiding in a first PR, it was just a "STATUS" so SEND_ERROR is a bit more intermediate and coherent with the rest of things than a FATAL_ERROR. And it will take more time to do quickly build ROOT if those things come in waves as @pcanal mentioned.
But if you all think it's better to fix all these in one go, I am ok with that too!
There was a problem hiding this comment.
In this particular case, one question is whether having in the cache cocoa=ON is harmful on non MacOS platform. i.e. Does it change the behavior of the CMakeList or is it a innocuous as using -Drandom_characters=ON?
If it is innocuous, there is no real reason to fail and a STATUS or WARNING is enough.
If it is not innocuous, then indeed it needs to be disabled by the user (and/or we need to fix our code to make it innocuous :) ) and because the default is NOT on, we can get to the error case only if the user explicitly said -Dcocoa=ON so it no longer quite fit under the quick build ROOT scheme but in a I need something specific from my build and thus in this particular case a FATAL_ERROR might be justified (a specific example would be: I wanted to build on MacOS but I copy/pasted my command line in a linux windows instead ... in this case the 'right' action is not to fix the build but to move to the 'right' window instead).
| message(STATUS "Switching off 'asimage' because neither 'x11' nor 'cocoa' are enabled") | ||
| set(asimage OFF CACHE BOOL "Disabled because neither x11 nor cocoa are enabled (${asimage_description})" FORCE) | ||
| message(SEND_ERROR "'asimage' needs either 'x11' or 'cocoa' enabled. Set -Dasimage=OFF") | ||
| list(APPEND HOTFIX_BUILD_FLAGS '-Dasimage=OFF') |
There was a problem hiding this comment.
I think this hotfix suggestion is not helpful. The user likely wanted image support.
There was a problem hiding this comment.
The hotfix suggestion was thought as a kind of partial backward compatibility. It's a DROP-IN replacement of whatever was automatic before.
Before: set(asimage OFF CACHE
Now: consider adding -Dasimage=OFF
But more than happy to remove if that full change of behavior is encouraged.
There was a problem hiding this comment.
In contrast with the cocoa case, asimage default to ON and thus we are now requiring that a user that does -Dx11=OFF on linux must also disable all the dependent packages.
I personally find that annoying (there is no way to know before hand what all the dependent package of x11 are) but I can understand the simplification it brings, so if we must make those simplification, we still must make it easy to disable. (However this is really a case that showcase the fact that the simplification we are going for is actually making life harder for our users - eg. if I understood correctly disabling x11 is now a 2 stage process :( ).
| ROOT_CHECK_CONNECTION("builtin_gsl") | ||
| if(NO_CONNECTION) | ||
| message(SEND_ERROR "No internet connection, check it or disable the 'builtin_gsl' option") | ||
| list(APPEND MISSING_PACKAGES 'GSL') | ||
| list(APPEND HOTFIX_BUILD_FLAGS '-Dbuiltin_gsl=OFF') | ||
| endif() |
There was a problem hiding this comment.
Use only
ROOT_ENSURE_CONNECTION("builtin_gsl")If builtin_gsl=Off, the code below does the right thing (i.e. putting the missing package into the list)
There was a problem hiding this comment.
I am confused ... in that section builtin_gsl is ON, so it wouldn't be added to the list, would it?
| if(NOT "${HOTFIX_BUILD_FLAGS}" STREQUAL "") | ||
| list(REMOVE_DUPLICATES HOTFIX_BUILD_FLAGS) | ||
| set(HOTFIX_BUILD_FLAGS_MESSAGE "Alternatively, a hotfix would be to add these flags to your CMake call:\n") | ||
|
|
||
| foreach(_item IN LISTS HOTFIX_BUILD_FLAGS) | ||
| string(APPEND HOTFIX_BUILD_FLAGS_MESSAGE " ${_item} \\\n") | ||
| endforeach() | ||
|
|
||
| # Remove final trailing backslash and newline | ||
| string(REGEX REPLACE "\\\\\n$" "" HOTFIX_BUILD_FLAGS_MESSAGE "${HOTFIX_BUILD_FLAGS_MESSAGE}") | ||
|
|
||
| message(FATAL_ERROR "${HOTFIX_BUILD_FLAGS_MESSAGE}") | ||
| endif() | ||
|
|
||
| # Now that builtins have passed this synchronization point, let's collect in a second pass | ||
| # additional error messages that can appear when using contradictory flags | ||
| unset(MISSING_PACKAGES) | ||
| unset(HOTFIX_BUILD_FLAGS_MESSAGE) | ||
|
|
There was a problem hiding this comment.
Hmm, I thought about this, but there is the risk that something later below when adding the add_subdirectory-builtins emits a FATAL_ERROR rather than a SEND_ERROR so that one does not reach to the final collecting message. Thus I thought it could be safer to have two synchronization points, one with very fast feedback to the user for the builtins, and then later errors below. Also, if first part fails, the second part will lead to many more warnings about missing things and overwhelm maybe the user. That's why I decided to have two distinct sections, the first one without add_subdirectory code (except for zlib), and the second with.
I agree with you, but I added this hotfix strategy to respond to @pcanal's observation: But if you take the final decision to not have that |
That goes in the opposite direction of https://github.com/root-project/root/pull/23020/changes that was already merged. Meaning: there was not such a list at all and code was short, but @pcanal requested changes that lead to adding that list of flags. To kind of preserve the backward compatibility (from the timing or logistics point of view) of the previous automatism behavior some people might have been relying on. From my side: I have no opinion, just let me know in what direction to continue ;) |
| #---If -Dshared=Off, prefer static libraries----------------------------------------- | ||
| if(NOT shared) | ||
| if(WINDOWS) | ||
| message(FATAL_ERROR "Option \"shared=Off\" not supported on Windows!") |
There was a problem hiding this comment.
| message(FATAL_ERROR "Option \"shared=Off\" not supported on Windows!") | |
| message(SEND_ERROR "Option \"shared=Off\" not supported on Windows!") |
To be coherent, there were complaints that errors come in waves, so this would be better since you would get all 'solutions' printed at the end, together
There was a problem hiding this comment.
I understand, but I think that we are coherent using FATAL_ERROR here. For things that are impossible, we might as well fail immediately.
For things that can be fixed by choosing to install a package, we can delay the message until we are able to name all packages that should be installed.
There was a problem hiding this comment.
I agree that FATAL_ERROR makes sense here.
I strong disagree. This PR might be indeed deal with disabling some component but the original need for |
I note that of the 3 ("install", "disable", "provide internet") we can only provide a hint for "disable" the other 2 are too user situation specific for us to give more than the vague instruction. However we may still want to improve the communication that the other 2 options are available. For example: This as the disadvantage of not being precise (what enables what). However one goal I have in mind (eg. the single list of |
OK, my bad. I didn't see it in time. 🙂 I would still try to be helpful without increasing the amount of code if possible (i.e. try to put repeated logic into a macro). In addition, I think that some failure modes are less likely than others, and therefore should be solved with a different strategy:
I would argue that 1. and 2. should be solved with a helpful list at the end of configure, whereas for 3., we could reasonably expect users to disable "by hand", i.e. reading the SEND_ERROR parts that occur while we configure. I imagine this to look like this: The no-internet case comes without a list, but the error message is still very actionable: This would mean that the above error message can be pulled into ROOT_CHECK_CONNECTION, and under every if(builtin_X)
ROOT_ENSURE_CONNECTION("builtin_xrootd")Is this acceptable / desirable @pcanal @ferdymercury ? |
All is acceptable to me as long as CACHE is not touched :) |
I think we are converging but there is 4th category (
In this case I argue that the most user friendly behavior is to auto-disable them ... but then what about the case where the user explicitly requested the dependent package ( |
Messing up with the cache is dangerous (calling to problems), and users often complain about not having the control since the build system changes what they just passed as flag to the command line. Instead, convert these automatism into a helpful error message that the user can use as a hotfix to modify their build flags. This prevents surprises, annoying behind-the-scene changes, as well as bugs. Follows up on the philosophy of #23020
Fixes https://its.cern.ch/jira/browse/ROOT-10743