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

ferdymercury · 2025-04-15T09:32:11Z

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.

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

Checklist:

tested changes locally
updated the docs (if necessary)

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.

github-actions · 2025-04-15T09:43:02Z

Test Results

17 files 17 suites 3d 23h 23m 1s ⏱️
2 744 tests 2 744 ✅ 0 💤 0 ❌
45 035 runs 45 035 ✅ 0 💤 0 ❌

Results for commit fb4fcc1.

♻️ This comment has been updated with latest results.

guitargeek

I'm all for it, thank you very much! And I think emitting a warning is a very good idea

guitargeek · 2025-04-15T11:41:45Z

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

ferdymercury · 2025-04-15T12:20:23Z

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 · 2025-04-15T12:44:15Z

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.

pcanal · 2025-04-15T12:48:00Z

cmake/modules/SearchInstalledSoftware.cmake

+    if(NOT mathmore)
+      message(WARNING "builtin_gls was enabled but mathmore wasn't")
+    endif()


Wouldn't that be 'better' or is mathmore already set on by default somewhere else?

Suggested change

if(NOT mathmore)

message(WARNING "builtin_gls was enabled but mathmore wasn't")

endif()

set(mathmore ON CACHE BOOL "Enabled because builtin_gls requested (${mathmore_description})")

if(NOT mathmore)

message(WARNING "builtin_gls was enabled but mathmore wasn't")

endif()

we have ROOT_BUILD_OPTION(mathmore OFF "Build libMathMore extended math library (requires GSL) [GPL]")
unless -Dall=ON then we have set(mathmore_defvalue ON)

But: tmva-cpu AND use_gsl_cblas could require builtin_gsl also.
So, it's not 100% legit to turn on mathmore just because builtin_gsl is ON ?

This goes with the general philosophy of this PR. Builtins are hierarchically lower than "features". You turn on or off features, which may or not use builtins. Turning ON builtins cannot turn ON features automatically, you will just get a warning (or a cmake error if you prefer), whereas turning ON features can force-enable builtins without further thoughts.

This goes with the general philosophy of this PR.

Indeed, I agree with the first part (disabling the feature should disable the feature) but not with the second part (enabling builtin without *also* enabling the feature fails to do what the user likely meant [there is editorial comment in the 2nd description :) :) ] )

pcanal · 2025-04-15T12:52:29Z

cmake/modules/SearchInstalledSoftware.cmake

 if(builtin_fftw3)
-  set(FFTW_VERSION 3.3.8)
-  message(STATUS "Downloading and building FFTW version ${FFTW_VERSION}")
-  set(FFTW_LIBRARIES ${CMAKE_BINARY_DIR}/lib/libfftw3.a)
-  ExternalProject_Add(
-    FFTW3
-    URL ${lcgpackages}/fftw-${FFTW_VERSION}.tar.gz
-    URL_HASH SHA256=6113262f6e92c5bd474f2875fa1b01054c4ad5040f6b0da7c03c98821d9ae303
-    INSTALL_DIR ${CMAKE_BINARY_DIR}
-    CONFIGURE_COMMAND ./configure --prefix=<INSTALL_DIR>
-    BUILD_COMMAND make CFLAGS=-fPIC
-    LOG_DOWNLOAD 1 LOG_CONFIGURE 1 LOG_BUILD 1 LOG_INSTALL 1 LOG_OUTPUT_ON_FAILURE 1
-    BUILD_IN_SOURCE 1
-    BUILD_BYPRODUCTS ${FFTW_LIBRARIES}
-    TIMEOUT 600
-  )
-  set(FFTW_INCLUDE_DIR ${CMAKE_BINARY_DIR}/include)
-  set(FFTW3_TARGET FFTW3)
-  set(fftw3 ON CACHE BOOL "Enabled because builtin_fftw3 requested (${fftw3_description})" FORCE)
+  if(NOT fftw3)
+    message(WARNING "fftw3 is OFF, hence ignoring the activated setting 'builtin_fftw3'")
+  else()


This does not feel quite right. This seems to now require both

-Dbuiltin_fftw3=ON -Dffw3=ON

to enable ffw3 with builtin or is some other part of the CMakeList allowing the simpler syntax?
I.e. like https://github.com/root-project/root/pull/18413/files#r2044449280 maybe we
need to move old line 944 here-ish, remove the FORCE and test afterwards?

cmake/modules/SearchInstalledSoftware.cmake

pcanal

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

ferdymercury · 2025-04-15T12:59:57Z

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 · 2025-04-15T15:04:17Z

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 · 2025-04-15T15:07:52Z

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 · 2025-04-16T12:20:19Z

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 · 2025-04-16T12:48:34Z

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 · 2025-04-16T13:12:06Z

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 · 2025-04-16T15:21:57Z

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 · 2025-04-20T06:23:20Z

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 · 2025-04-21T17:15:10Z

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 · 2025-04-22T06:47:42Z

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.

…f features

ferdymercury · 2025-04-22T07:15:37Z

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: #14311 (comment)

guitargeek · 2025-04-22T07:19:57Z

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 · 2025-04-22T16:41:01Z

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 · 2025-04-22T17:02:40Z

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

cmake/modules/SearchInstalledSoftware.cmake

ferdymercury · 2025-04-22T17:50:18Z

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.

Changes implemented in separate v2 of this PR

ferdymercury · 2025-04-23T15:46:28Z

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

ferdymercury requested review from amadio, guitargeek and hageboeck April 15, 2025 09:32

ferdymercury requested a review from bellenot as a code owner April 15, 2025 09:32

[cmake] fix typo in fitsio

b2c0e7e

ferdymercury changed the title ~~[cmake] feature=OFF has hierarchical priority over builtin_feature=ON~~ [cmake] feature=OFF should have hierarchical priority over (cached) builtin_feature=ON Apr 15, 2025

ferdymercury changed the title ~~[cmake] feature=OFF should have hierarchical priority over (cached) builtin_feature=ON~~ [cmake] feature=OFF should have hierarchical priority over (cached, or not) builtin_feature=ON Apr 15, 2025

guitargeek approved these changes Apr 15, 2025

View reviewed changes

pcanal reviewed Apr 15, 2025

View reviewed changes

cmake/modules/SearchInstalledSoftware.cmake Show resolved Hide resolved

pcanal previously requested changes Apr 15, 2025

View reviewed changes

ferdymercury added this to the 6.38.00 milestone Apr 15, 2025

[cmake] fftw3 ON is allowed to turn on builtin_fftw3 as in the rest o…

86e52fe

…f features

ferdymercury commented Apr 22, 2025

View reviewed changes

[cmake] convert warnings into errors if a conflict is found

fb4fcc1

ferdymercury mentioned this pull request Apr 22, 2025

[cmake] explicit feature=OFF should have hierarchical priority over cached or explicit builtin_feature=ON v2 #18467

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ferdymercury commented Apr 15, 2025 •

edited

Loading

github-actions bot commented Apr 15, 2025 •

edited

Loading

guitargeek left a comment

guitargeek commented Apr 15, 2025

ferdymercury commented Apr 15, 2025 •

edited

Loading

pcanal commented Apr 15, 2025

pcanal Apr 15, 2025

ferdymercury Apr 15, 2025

pcanal Apr 21, 2025

pcanal Apr 15, 2025

pcanal left a comment

ferdymercury commented Apr 15, 2025

hahnjo commented Apr 15, 2025 •

edited

Loading

ferdymercury commented Apr 15, 2025

pcanal commented Apr 16, 2025

pcanal commented Apr 16, 2025 •

edited

Loading

ferdymercury commented Apr 16, 2025

pcanal commented Apr 16, 2025

guitargeek commented Apr 20, 2025 •

edited

Loading

pcanal commented Apr 21, 2025

guitargeek commented Apr 22, 2025 •

edited

Loading

ferdymercury commented Apr 22, 2025 •

edited

Loading

guitargeek commented Apr 22, 2025

pcanal commented Apr 22, 2025

pcanal commented Apr 22, 2025 •

edited

Loading

ferdymercury commented Apr 22, 2025

ferdymercury commented Apr 23, 2025

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

Are you sure you want to change the base?

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

Conversation

ferdymercury commented Apr 15, 2025 • edited Loading

This Pull request:

Changes or fixes:

Checklist:

github-actions bot commented Apr 15, 2025 • edited Loading

Test Results

guitargeek left a comment

Choose a reason for hiding this comment

guitargeek commented Apr 15, 2025

ferdymercury commented Apr 15, 2025 • edited Loading

pcanal commented Apr 15, 2025

pcanal Apr 15, 2025

Choose a reason for hiding this comment

ferdymercury Apr 15, 2025

Choose a reason for hiding this comment

pcanal Apr 21, 2025

Choose a reason for hiding this comment

pcanal Apr 15, 2025

Choose a reason for hiding this comment

pcanal left a comment

Choose a reason for hiding this comment

ferdymercury commented Apr 15, 2025

hahnjo commented Apr 15, 2025 • edited Loading

ferdymercury commented Apr 15, 2025

pcanal commented Apr 16, 2025

pcanal commented Apr 16, 2025 • edited Loading

ferdymercury commented Apr 16, 2025

pcanal commented Apr 16, 2025

guitargeek commented Apr 20, 2025 • edited Loading

pcanal commented Apr 21, 2025

guitargeek commented Apr 22, 2025 • edited Loading

ferdymercury commented Apr 22, 2025 • edited Loading

guitargeek commented Apr 22, 2025

pcanal commented Apr 22, 2025

pcanal commented Apr 22, 2025 • edited Loading

ferdymercury commented Apr 22, 2025

ferdymercury commented Apr 23, 2025

ferdymercury commented Apr 15, 2025 •

edited

Loading

github-actions bot commented Apr 15, 2025 •

edited

Loading

ferdymercury commented Apr 15, 2025 •

edited

Loading

hahnjo commented Apr 15, 2025 •

edited

Loading

pcanal commented Apr 16, 2025 •

edited

Loading

guitargeek commented Apr 20, 2025 •

edited

Loading

guitargeek commented Apr 22, 2025 •

edited

Loading

ferdymercury commented Apr 22, 2025 •

edited

Loading

pcanal commented Apr 22, 2025 •

edited

Loading