Skip to content

[cmake] Do not auto-turn OFF feature or auto-turn ON builtin_option - #23020

Merged
pcanal merged 6 commits into
root-project:masterfrom
ferdymercury:failondef2
Aug 17, 2026
Merged

[cmake] Do not auto-turn OFF feature or auto-turn ON builtin_option#23020
pcanal merged 6 commits into
root-project:masterfrom
ferdymercury:failondef2

Conversation

@ferdymercury

@ferdymercury ferdymercury commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

for easy-to-install dependencies (via homebrew, apt-get)

when dependency is missing in the system. Instead, force user to install it via package manager, or to turn feature purposely OFF by hand.

Spit-off from #23015 as suggested by @guitargeek

This goes totally in-line with what's already done for asimage, zlib, etc.

Comment thread cmake/modules/SearchInstalledSoftware.cmake Outdated
Comment thread cmake/modules/SearchInstalledSoftware.cmake Outdated
Comment thread README/ReleaseNotes/v642/index.md Outdated
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Test Results

    23 files      23 suites   3d 18h 53m 34s ⏱️
 3 859 tests  3 858 ✅ 0 💤 1 ❌
79 522 runs  79 520 ✅ 1 💤 1 ❌

For more details on these failures, see this check.

Results for commit 19848e1.

♻️ This comment has been updated with latest results.

@ferdymercury
ferdymercury force-pushed the failondef2 branch 2 times, most recently from 98c555c to 375b1cb Compare August 7, 2026 15:46
@pcanal

pcanal commented Aug 7, 2026

Copy link
Copy Markdown
Member

most ROOT builtins already call ROOT_FIND_REQUIRED_DEP which purposely bypasses this flag,

After some usage, I find this change to be indeed more pedantic (useful for system maintainer) but also annoyingly-so 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).

so I am not (yet?) convinced that completely removing the feature (auto-turn on) is desirable.

@ferdymercury

ferdymercury commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

I think AI could do that parsing and -Dbuiltin=On for you to paste on the terminal. One could also improve the printed error messages so that Root already proposes -Dbuiltin1=ON ... In a one liner rather than scattered as now

now installing ROOT on a system that does not already have the required builtin has to go through a step of parsing

Can't you install instead the dependencies via homebrew or apt? So that you don't need to parse anything, just copy paste packages.txt from rootci images?

I think more developer time is spent on maintaining those many code branches (and bugs) than the time spent in writing by hand built-in On.

@pcanal

pcanal commented Aug 10, 2026

Copy link
Copy Markdown
Member

Can't you install instead the dependencies via homebrew or apt? So that you don't need to parse anything, just copy paste packages.txt from rootci images?

I am using a system that has neither homebrew nor apt.

I think AI could do that parsing and -Dbuiltin=On for you to paste on the terminal. One could also improve the printed error messages so that Root already proposes -Dbuiltin1=ON ... In a one liner rather than scattered as now

I suspect that would help (albeit in my latest attempt they came in waves (i.e. not all were printed in the first run))

than the time spent in writing by hand built-in On.

This is hard to quantify since this is about end-user-but-I-still-build-ROOT-locally use case.

@silverweed

silverweed commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

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

I deem this to be the totally-normal process that one should expect whenever one wants to compile a package themselves...

@ferdymercury

Copy link
Copy Markdown
Collaborator Author

I suspect that would help (albeit in my latest attempt they came in waves (i.e. not all were printed in the first run))

Could you re-check now ? All messages should come all together in one wave, and the last line should be the hotfix that you could copy paste verbatim to have the required builtin_xx = ON flags.

@ferdymercury ferdymercury added the skip code analysis Skip the code analysis CI steps for this PR, including verifying clang-formatting and running Ruff. label Aug 11, 2026
@ferdymercury

Copy link
Copy Markdown
Collaborator Author

Failures seem unrelated

@pcanal

pcanal commented Aug 13, 2026

Copy link
Copy Markdown
Member

I suspect that would help (albeit in my latest attempt they came in waves (i.e. not all were printed in the first run))

Could you re-check now ? All messages should come all together in one wave, and the last line should be the hotfix that you could copy paste verbatim to have the required builtin_xx = ON flags.

I think this is a good idea and we just need to tweak the printout. For me it came out as:

CMake Error at cmake/modules/SearchInstalledSoftware.cmake:286 (message):
  Alternatively, a hotfix would be to add these flags to your CMake call:
  '-Dbuiltin_lz4=ON';'-Dbuiltin_lzma=ON';'-Dbuiltin_zstd=ON';'-Dbuiltin_xxhash=ON';'-Dbuiltin_gif=ON';'-Dbuiltin_jpeg=ON';'-Dbuiltin_png=ON';'-Dbuiltin_tiff=ON';'-Dbuiltin_nlohmannjson=ON';'-Dbuiltin_freetype=ON';'-Dbuiltin_gl2ps=ON';'-Dbuiltin_ftgl=ON';'-Dbuiltin_openssl=ON';'-Dssl=OFF';'-Dbuiltin_civetweb=ON';'-Dbuiltin_vdt=ON';'-Dbuiltin_cfitsio=ON';'-Dbuiltin_xrootd=ON';'-Dbuiltin_tbb=ON';'-Dbuiltin_pcre=ON'
Call Stack (most recent call first):
  CMakeLists.txt:136 (include)

where we just need to ensure the ; are replaced by spaces.

Thanks.

for easy-to-install dependencies (via homebrew, apt-get)

when dependency is missing in the system. Instead, force user to install it via package manager, or to turn feature purposely OFF.

[nfc] improve error messages

Co-authored-by: ferdymercury <ferdymercury@users.noreply.github.com>
to speedup workflow as suggested by pcanal
Use consistently everywhere send_error before summary fatal_error

provide a useful hotfix flag for CMake as suggested by pcanal
@ferdymercury

Copy link
Copy Markdown
Collaborator Author

Right... fixed now

@ferdymercury
ferdymercury requested a review from pcanal August 13, 2026 17:31
Comment thread cmake/modules/SearchInstalledSoftware.cmake Outdated
Co-authored-by: Philippe Canal <pcanal@fnal.gov>
@pcanal pcanal closed this Aug 14, 2026
@pcanal pcanal reopened this Aug 14, 2026
@ferdymercury
ferdymercury requested a review from pcanal August 14, 2026 18:11
@ferdymercury

Copy link
Copy Markdown
Collaborator Author

Looking green :)

@pcanal

pcanal commented Aug 17, 2026

Copy link
Copy Markdown
Member

Yes, very green! Thanks. (Although the 'Checks' link/tab has a stale version of the "Test Results" which shows the (expected) transient failure in roottest-root-io-cpp11Containers-unorderedMap)

@pcanal pcanal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank!

@pcanal
pcanal merged commit 2865a39 into root-project:master Aug 17, 2026
114 of 119 checks passed
@ferdymercury
ferdymercury deleted the failondef2 branch August 17, 2026 21:23
ferdymercury added a commit to ferdymercury/root that referenced this pull request Aug 18, 2026
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:squash on merge skip code analysis Skip the code analysis CI steps for this PR, including verifying clang-formatting and running Ruff.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants