[cmake] Do not auto force-enable or force-disable user CMake build options

[ferdymercury]

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

[github-actions]

Test Results

    23 files      23 suites   3d 17h 13m 49s ⏱️
 3 855 tests  3 854 ✅ 0 💤 1 ❌
79 480 runs  79 479 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 008da98.

♻️ This comment has been updated with latest results.

[pcanal]

@ferdymercury Can you add a comment to close the now seemingly superseded #18413 explaining where we actually ended up?

[ferdymercury]

Sure, comment added: #18413 (comment)
And CI errors fixed now.

[ferdymercury]

(asan failure seems unrelated)

[hageboeck]

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_FLAGS list 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.

[hageboeck]

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.

[ferdymercury]

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!

[pcanal]

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).

[hageboeck]

I think this hotfix suggestion is not helpful. The user likely wanted image support.

[ferdymercury]

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.

[pcanal]

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 :( ).

[ferdymercury]

• 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.

I agree with you, but I added this hotfix strategy to respond to @pcanal's observation: for the 'quickly build ROOT on this system for my private usage' (I.e. now installing ROOT on a system that does not already have the required builtin has to go through a step of parsing (by hand or mechanically) the output of cmake to find the --builtin- are that needed ... I find this onerous for a private installation) #23020 (comment)

But if you take the final decision to not have that most quickest solution to build, I am more than happy to remove all that extra code turning components OFF :)

[ferdymercury]

• you will find that the HOTFIX_BUILD_FLAGS list will be mostly empty, so to shorten and simplify the code, I would remove it entirely.

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 ;)

[pcanal]

If you agree to the above, you will find that the HOTFIX_BUILD_FLAGS list will be mostly empty, so to shorten and simplify the code, I would remove it entirely

I strong disagree. This PR might be indeed deal with disabling some component but the original need for HOTFIX_BUILD_FLAGS was to list to flag needed to enable the not-installed builtins. Those are necessary to install ROOT on a system where those are not available easily (MacOS without homebrew/macport, system without admin privilege, etc.) and thus the HOTFIX_BUILD_FLAGS was a requirement to allow moving from the "builtins are on by default when missing" to "they must be enabled by hand".

[pcanal]

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.

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:

To enable all the requested option you could either provide an internet connection or install the following packages:
   abc
   xyz
or you can disable the options with:
   -Dclad=OFF

This as the disadvantage of not being precise (what enables what).

However one goal I have in mind (eg. the single list of -Dbuiltin..=ON) is to make a quick build (i.e. I might not care that all the features are on but I need something functional) easy enough (i.e. the previous state was that it was easy - but admittedly possibly confusing on what was build from where). With my reading of the alternative proposals (eg no more hotfix list), I am afraid the quick build would not be so annoying labor intensive as to be useless (but not quick at all) (unless you already pre-installed everything you need).

[hageboeck]

• you will find that the HOTFIX_BUILD_FLAGS list will be mostly empty, so to shorten and simplify the code, I would remove it entirely.

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.

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:

1. [Frequent; by now ROOT's preferred MO] A user wants to build the minimum possible, so they want a list of all required packages to install.
2. [Frequent] A user doesn't want to (or doesn't have permissions to) install packages, so they want a list of builtins they need to enable.
3. [Rare] We can't satisfy any of the above, because there's no connection. In this case, one has to figure out which options should be disabled.

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:

[...]
cmake error: The build option -D<opt> requires <X>
[...]
cmake error: The build option -D<opt2> requires <Y>
[...]
Please install the following packages or provide CMake search paths for them:
 X Y
Alternatively, enable the builtin versions:
 -Dbuiltin_X=On -Dbuiltin_Y=On

The no-internet case comes without a list, but the error message is still very actionable:

$ cmake -Dbuiltin_X=On [...] <src>
[...]
cmake error: The builtin package -Dbuiltin_X requires an internet connection.
[...]

This would mean that the above error message can be pulled into ROOT_CHECK_CONNECTION, and under every if(builtin_package) we only have one single line:

if(builtin_X)
  ROOT_ENSURE_CONNECTION("builtin_xrootd")

Is this acceptable / desirable @pcanal @ferdymercury ?

[ferdymercury]

Is this acceptable / desirable @pcanal @ferdymercury ?

All is acceptable to me as long as CACHE is not touched :)
So my opinion does not count regarding what messaging strategy is best, I'll follow what you decide

[pcanal]

1. [Frequent; by now ROOT's preferred MO] A user wants to build the minimum possible, so they want a list of all required packages to install.
2. [Frequent] A user doesn't want to (or doesn't have permissions to) install packages, so they want a list of builtins they need to enable.
3. [Rare] We can't satisfy any of the above, because there's no connection. In this case, one has to figure out which options should be disabled.

I think we are converging but there is 4th category (asimage)

4. [Common] A user disable a package (x11) but not the other default packages that depends on it (asimage).

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 (-Dasimage=ON) ... Maybe for those package we need to have them default to a 3rd option (auto) and then hard fail early if they are ON when the package they depend (x11) on is OFF.

[ferdymercury]

@hageboeck I tried to simplify some checks regarding Internet connection as you suggested.

With these, SearchInstalledSoftware remains basically equal length:

+226-224 Lines changed: 226 additions & 224 deletions

There is probably room for further optimization and message refinement, but I think it makes sense to first decide /agree on the behavior, and a subsequent PR can optimize / refine the messaging?

In this case I argue that the most user friendly behavior is to auto-disable them

I somewhat disagree, the whole point of the PR was to reduce code maintenance and reported 7-year-old bugs due to messing up with the cache. My opinion is that it's more user friendly to have a build system that is robust and has no bugs even if it's a bit pedantic, than an automatic build system that has an annoying bug that no one is able to solve for 7 years. Touching the cache for a single edge case is incoherent and invites for future additions again of touching the cache, my proposal was to remove all hints to it.
My hypothesis / claim is that the manpower involved in developing and maintaining a robust automatic and complicated mechanism for asimage / x11 is larger than the summatory of time spent in copy-pasting the solution from the terminal times number of occurrences times number of developers.
In any case, I fixed it so that it's not a two-step process.