[cmake] feature=OFF should have hierarchical priority over (cached, or not) builtin_feature=ON

[ferdymercury]

This Pull request:

Changes or fixes:

Fixes https://its.cern.ch/jira/browse/ROOT-10743

feature=ON can force-enable builtin_feature=OFF, but not the other way round, a warning is issued in that case now instead of silently enabling.

This prevents automatic enablings of features that the user can not disable unless he writes feature=OFF builtin_feature=OFF.

I've been often annoyed about using -Dfftw3=OFF and cmake overriding it anyway. If this new behavior is not wanted, then I would suggest closing the associated JIRA Issue.

Related:

• https://its.cern.ch/jira/browse/ROOT-9385

Checklist:

• [x] tested changes locally
• [ ] updated the docs (if necessary)

[github-actions[bot]]

Test Results

    18 files      18 suites   4d 0h 58m 33s ⏱️  2 731 tests  2 731 ✅ 0 💤 0 ❌ 47 771 runs  47 771 ✅ 0 💤 0 ❌

Results for commit 7030b329.

:recycle: This comment has been updated with latest results.

[guitargeek]

I think this has to be mentioned in the release notes though

[ferdymercury]

Thanks Jonas for the review! 6.36 relnotes or 6.38? I will do that in a separate PR, to avoid re-running the CI.

[pcanal]

Thanks Jonas for the review! 6.36 relnotes or 6.38? I will do that in a separate PR, to avoid re-running the CI.

This should be only considered for v6.38. Besides potential debate on the content, it is highly likely to cause unforeseen (but not necessarily undesirable but still ...) visible (or not) consequences for user builds.

[ferdymercury]

Unless I am mistaken, the patch does not seem to accomplish exactly what the title hints at.

Thanks for the review!

The proposed behavior here is as follows: Calling just -Dbuiltin_fftw3=ON, one will get a warning saying that fftw3=OFF, so it will be ignored. The user needs to call both. On the other hand, if one sets fftw3=ON, CMake can auto-set internally builtin_fftw3=ON if it does not find the system-wide package. Finally, if I want to turn it back of, I just need to set fftw3=OFF, and not like before fftw3=OFF builtin_fftw3=OFF.

So the debate is, do we want 2 flags for turning off vs 2 flags for turning on.

I do not know if there is a third way allowing for "remembering" if builtin_fftw3 was set by a previous call to CMake by the user, or by auto-finding when not getting system packages, or actively by the user.

[hahnjo]

Note that this sounds very risky to do one week before branching. There is simply not enough time to test all possible combinations and make sure they work correctly.

[ferdymercury]

Note that this sounds very risk to do one week before branching. There is simply not enough time to test all possible combinations and make sure they work correctly.

Yes, I fully agree, 6.38 seems more reasonable.

[pcanal]

I do not know if there is a third way allowing for "remembering" if builtin_fftw3 was set by a previous call to CMake by the user

There is :). (not necessarily easy code but there is)

[pcanal]

So the debate is, do we want 2 flags for turning off vs 2 flags for turning on.

An important side note is that the proposed behavior will (likely) break existing build scripts (i.e. this is a breaking change).

In attempt to clarify my mind on this, I started this table to list the current behavior, proposed behavior and comments.

#	CMakeCache State	fftw3	builtin_fftw3	Resulting State	Proposed State	Comment
1	(empty)	ON	(none)	fftw3=ON builtin_fftw3=ON	(same)
2	(empty)	(none)	ON	fftw3=ON builtin_fftw3=ON	fftw3=OFF builtin_fftw3=ON	Why not keep turning on fftw3 in that case?
3	fftw3=ON builtin_fftw3=ON	OFF	(none)	fftw3=ON builtin_fftw3=ON	fftw3=OFF builtin_fftw3=ON	The existing situation is surprising.
4	fftw3=OFF builtin_fftw3=ON (currently impossible)	ON	(none)	fftw3=ON builtin_fftw3=ON	fftw3=ON builtin_fftw3=ON

I am not sure it helps or if we should flush it out.

I does denotes in the 2nd row, the breaking change ... which I believe is not necessary and could be kept as is while still addressing the bug shown in the 3rd row.

[ferdymercury]

Why not keep turning on fftw3 in that case?

I see your point, but it's because it can be (silently) misleading. Or at least it should print a warning in my opinion.

Reason: sometimes I have a very long-list of build options:

asimage_tiff=OFF
builtin_cfitsio=ON
builtin_cppzmq=ON
builtin_davix=ON
builtin_fftw3=ON
builtin_freetype=ON
builtin_ftgl=ON
builtin_gif=ON
builtin_gl2ps=ON
builtin_glew=ON
builtin_gsl=ON
builtin_gtest=ON
builtin_jpeg=ON
builtin_lz4=ON
builtin_lzma=ON
builtin_nlohmannjson=ON
builtin_openssl=ON
builtin_pcre=ON
builtin_tbb=ON
builtin_vc=ON
builtin_vdt=ON
builtin_veccore=ON
builtin_xrootd=ON
builtin_xxhash=ON
builtin_zeromq=ON
builtin_zstd=ON
ccache=ON
cocoa=ON
fftw3=ON
fortran=OFF
pythia8=OFF
test_distrdf_dask=ON
test_distrdf_pyspark=ON
tmva-sofie=ON
x11=OFF

Then I want to disable fftw3, so I go to line fftw3 and completely remove it since I know that the default value is OFF according to the webpage. After configuring make I see fftw3 is still ON, so I am confused. I run again and same result. Then I re-check everything and find out, that there is also builtin_fftw3 somewhere up there that I need not just remove, but explictly set to OFF.

So my proposal was to make them hierarchical, which makes thinks more understandable and controllable (less silent automatic magic behind). I understand though the side effects, so if that's not wanted, then in my opinion it would be nice to print a CMake warning: "Force enabling fftw3=ON since builtin was requested".

[pcanal]

I agree that the current situation is broken (need the change in row 3) but I still think it can be achieved while not making the change in row 2.

[guitargeek]

But the change For row 2 is the whole point of this PR as Ferdy said, and personally I agree with his opinion and the user story in his previous comment is valid. Why you don't think that the proposed behavior is better, besides backwards compatibility concerns? Fixing row 3 is more of a niche case, not sure if it's worth to do something about it...

Edit: maybe I'm biased because I'm always rebuilding from scratch when changing configuration flags. So for Ferdy's story, case 3 automatically becomes case 2 again :)

[pcanal]

Why you don't think that the proposed behavior is better, besides backwards compatibility concerns?

Because it unnecessarily (as far I can tell) breaks backward compatibility (and the proposed code in the PR does not quite introduce "hierarchical priority" (which would be backward compatible)). (And in row 2, the state fftw3=OFF builtin_fftw3=ON seems like an unintentional of 'waste' of time - why would a user request the builtin fftw3 ... just for it to sit unused? -- if the user say fftw3=OFF explicitly then fine, this is a what they asked for but if they did not specify it, it makes no sense (to me))

[guitargeek]

proposed code in the PR does not quite introduce "hierarchical priority"

As far as I understand, it does. The right hierarchical dependency that we want is the following. So far: xrootd=ON implies xrootd_fftw3=ON (if not found on the system), and builtin_xrootd=ON implied xrootd=ON. So they are "symmetric". We want that xrootd_fftw3=ON does not imply anything anymore, but instead xrootd=OFF implies xrootd_fftw3=OFF. This introduces a clear hierarchy on which options value implies which other option.

And in row 2, the state fftw3=OFF builtin_fftw3=ON seems like an unintentional of 'waste' of time - why would a user request the builtin fftw3 ... just for it to sit unused? -- if the user say fftw3=OFF explicitly then fine, this is a what they asked for but if they did not specify it, it makes no sense (to me)

This is exactly what this PR is supposed to address, no @ferdymercury? Maybe what makes it so confusing that so far, there is no clear policy on how option and builtin_option interact, they all do it differently. For example, xrootd=ON implies xrootd_fftw3=ON if xrootd is not found on the system, like I mentioned before. However, fftw3=ON does not imply builtin_fftw3=ON, but still builtin_fftw3=ON implies fftw3=ON, which is actually the inverted hierarchy of what this PR proposes! Doesn't make sense to me.

Because it unnecessarily (as far I can tell) breaks backward compatibility

The configuration is so random right now that I would not prioritize backwards compatibility but instead consistency and clear documented policies from now on.

[ferdymercury]

This introduces a clear hierarchy on which options value implies which other option.

Yes, exactly, that was my proposal. And independently on whether the option was cached from a previous run or not. It does though 'respect' the initial hard-coded default values. So if fftw3 were ON by default, you could just specify builtin_fftw3=ON, no need for both.

still builtin_fftw3=ON implies fftw3=ON

Are you sure? I just thought that this snippet prevented that:

if(builtin_fftw3)
  if(NOT fftw3)
    message(WARNING "fftw3 is OFF, hence ignoring the activated setting 'builtin_fftw3'")

However, fftw3=ON does not imply builtin_fftw3=ON

I have fixed this issue in the latest commit, thanks for noticing. (I overlooked this).

I would not prioritize backwards compatibility but instead consistency and clear documented policies from now on.

In my opinion, if keeping backward compatibility can be done with let's say 20 lines of extra total CMake code (I am not sure how to do that), then I agree with @pcanal's assessment. If however it requires very complicated CMake code that is hard to maintain and makes the automatic ROOT-CMake procedures less hard to disentangle / reproduce, then I agree with @guitargeek that having a simplified decision hygiene, a bit simple and dumb, but giving you a bit pedantic warnings so that it's supereasy to follow, would be better even at the price of breaking backward compatibility (with a clear warning of what to fix when calling your build script). And it's easier to maintain if someone else adds in the future a new ROOT build option. Related discussion: https://github.com/root-project/root/pull/14311#issue-2071310206

[guitargeek]

Are you sure? I just thought that this snippet prevented that:

Sorry I was talking about the inconsistent behavior of ROOT master, not this PR

[pcanal]

In my opinion, if keeping backward compatibility can be done with let's say 20 lines of extra total CMake code (I am not sure how to do that)

It looks like it can be done with a couple of lines (per case of builtin). For example: https://github.com/root-project/root/pull/18413/files#r2044449280 might be sufficient (and if not it can still be done with a couple more lines).

[pcanal]

but still builtin_fftw3=ON implies fftw3=ON

I don't think that is a problem. The 'only' problem I see is that the current code does the wrong thing in that it does more than that and actually does builtin_fftw3=ON **forces** fftw3=ON (and this is a serious problem).

I extend the table to include all configurations for a from scratch configuration and in the proposed state column is now included 'my' recommendation:

#	CMakeCache State	fftw3	builtin_fftw3	Resulting State	Proposed State	Comment
1	(empty)	ON	(none)	fftw3=ON builtin_fftw3=ON	(same)
2	(empty)	ON	OFF	fftw3=ON builtin_fftw3=OFF	(same)
3	(empty)	ON	ON	fftw3=ON builtin_fftw3=ON	(same)
4	(empty)	(none)	(none)	fftw3=OFF builtin_fftw3=OFF	(same)
5	(empty)	(none)	OFF	fftw3=OFF builtin_fftw3=OFF	(same)
6	(empty)	(none)	ON	fftw3=ON builtin_fftw3=ON	(same)	the debated configuration
7	(empty)	OFF	(none)	fftw3=OFF builtin_fftw3=OFF	(same)
8	(empty)	OFF	OFF	fftw3=OFF builtin_fftw3=OFF	(same)
9	(empty)	OFF	ON	fftw3=ON builtin_fftw3=ON	fftw3=OFF builtin_fftw3=ON	the problem
10	fftw3=ON builtin_fftw3=ON	OFF	(none)	fftw3=ON builtin_fftw3=ON	fftw3=OFF builtin_fftw3=ON	The existing situation is surprising.
11	fftw3=OFF builtin_fftw3=ON (currently impossible)	ON	(none)	fftw3=ON builtin_fftw3=ON	fftw3=ON builtin_fftw3=ON

As a side note, if we follow the recommendation from the PR, the existing configuration:

cmake -Dbuiltin_fftw3=ON $root_src

will 'silently' no longer build the fftw3 library (i.e. an experiment could build the entire stack release it, only to have their user discover the miss configuration sometime later). So if we go that route we actually need to detect the case and error out. Consequently the amount of code (times the number of builtin) is the same order of magnitude for both solution.

Maybe what makes it so confusing that so far, there is no clear policy on how option and builtin_option interact, they all do it differently.

I agree, this is also bad and need to either be documented or 'fixed' (make them all do the same).

[ferdymercury]

discover the miss configuration sometime later).

Side note, this also happens if they have a missing Internet connection and fail-on-missing is off.

So if we go that route we actually need to detect the case and error out. Consequently the amount of code (times the number of builtin) is the same order of magnitude for both solution.

Agreed. I have transformed the warnings into errors now. So I guess this would be plan A, and plan B would be remove the FATAL_ERROR message and soft-enable the feature via the cache as you mentioned.

[ferdymercury]

Related issue: https://its.cern.ch/jira/projects/ROOT/issues/ROOT-9385

[ferdymercury]

Side note: the unuran option kind of follows the philosophy of this PR since 2018: See commit by @amadio 3e60764f133218b6938e5aa4986de760e8f058d9 to solve the last item of https://its.cern.ch/jira/browse/ROOT-9385 which is kind of a 'dupe' of this one.

[silverweed]

@ferdymercury should this PR be closed, considering that there is a v2 of it, or is it still needed for something?

[ferdymercury]

@ferdymercury should this PR be closed, considering that there is a v2 of it, or is it still needed for something?

I think that first, the ROOT team should decide what's the best way to follow of these three options.

a) Leave things as they are and close the JIRA issue as won't fix and document this limitation. b) Change behavior by simplifying things a lot as done in this PR (v1). Very clear to follow and easy to maintain in the future, it forces though the user to be a bit pedantic when passing some options since some of the automation logic is removed. Even if it might interfere with some build scripts, it's very easy to fix them and people might like the new clear pedantic logic rather than the current magic OFF does not turn OFF. Note: the current behavior is undocumented, broken and unconsistent. For example, builtin_unuran does not turn unuran ON. So does this PR really break behavior? Since there are two simultaneous philosophies, we are choosing one to be consistent, so it could be seen more as a fix of UndefinedBehavior rather than as a breaking change. c) Do not compromise the complex automatic logic. Try at the same time to fix the bug ad suggested by pcanal. This needs advanced complex Cmake tricks. That's what I attempted in v2 of this PR. But I did not succeed, see discussion https://github.com/root-project/root/pull/18467#issuecomment-2823616362 Maybe https://stackoverflow.com/questions/43542381/set-a-cmake-variable-if-it-is-not-changed-by-the-user helps but I think an expert needs to take over here. d) Like b) but adding first a deprecation warning and doing the change in 6.40. I don't really like this option since the current behavior is still broken or inconsistent or undocumented, so why deprecating it, if the solution is rather easy if a user relied on the auto-turn-on.

So before closing v1 or v2 or both, a decision or analysis of cost benefit ratio is needed I guess.

This issue reminds me a bit of https://github.com/root-project/root/pull/16410

In my view in both issues, if the code complexity/maintenance/clarity is reduced, then it's worth to nudge the users to modify their scripts to add auto or feature=ON.