Skip to content

[cmake] make fail-on-missing=ON the only behavior, ignore user setting and deprecate build option - #23015

Draft
ferdymercury wants to merge 1 commit into
root-project:masterfrom
ferdymercury:failondef
Draft

[cmake] make fail-on-missing=ON the only behavior, ignore user setting and deprecate build option#23015
ferdymercury wants to merge 1 commit into
root-project:masterfrom
ferdymercury:failondef

Conversation

@ferdymercury

Copy link
Copy Markdown
Collaborator

This Pull request:

Changes or fixes:

fail-on-missing flag no longer makes any sense: most ROOT builtins already call ROOT_FIND_REQUIRED_DEP which purposely bypasses this flag, so users might be confused about this hybrid undocumented behavior. Now all builtins can be installed via APT or via homebrew, so it's less of an issue.

This allows a huge simplification of search-installed-software, and easens code maintenance and bug fixing of the build system for the developers. The only downside is that some users might need to type a bit more consciously what is ON and OFF. Which some might even welcome, others complain.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

@guitargeek

Copy link
Copy Markdown
Contributor

Thanks for the initiative! As I see it, the suggested change splits into an uncontroversial and a controversial part.

The uncontroversial part: removing the fail-on-missing=OFF path for all dependencies that back opt-in features, like arrow, which are OFF by default. If one explicitly selects arrow=ON at build time (or implies it with all=ON), then it's pretty annoying if the CMake code disables the feature and proceeds to build ROOT if find_package(Arrow) didn't succeed.

The controversial part: maybe some people relied on the automatic disabling of default features if the dependencies that backed them were not available. Or they relied on the fallback to the builtin version if available.

I think your suggestion would fly if you split it in two parts: first a PR with the uncontroversial part that should sail through, and then a PR for the remaining "controversial" changes.

Would that make sense to you?

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Test Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit 6a0f346.

♻️ This comment has been updated with latest results.

@ferdymercury

Copy link
Copy Markdown
Collaborator Author

Thanks for the feedback !

maybe some people relied on the automatic disabling of default features if the dependencies that backed them were not available.

On the other hand, this is already broken now:

  • If you specify asimage=ON, now it always fail due to the use of ROOT_FIND_REQUIRED_DEP, whereas some time ago, it was disabling it.
  • If you specify opengl=ON, same story

So this PR is making that current behavior consistent through all the build options.

Yes, I could split the PR in two if that helps, but the gain is not high since users can not rely any more on that historical behavior. I prefer annoying (I have to turn things ON/OFF by hand) but coherent behavior (every option behaves likewise) than heterogeneous one (why does one dependency autodisable and another one does not).

What do you think?

@ferdymercury
ferdymercury marked this pull request as ready for review August 5, 2026 06:21
@guitargeek

guitargeek commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

I was suggesting to split this PR not for the users, but for the review process. If the uncontroversial part is merged first, then the diff of the "controversial" PR that removes fail-on-missing becomes smaller, and it will be easier to sell this behavior change to everyone.

Personally I'm all for this change though!

Comment thread cmake/modules/SearchInstalledSoftware.cmake Outdated
@ferdymercury

Copy link
Copy Markdown
Collaborator Author

, and it will be easier to sell this behavior change to everyone.

Make sense, will do, thanks

…g and deprecate flag

[nfc] mention change in release notes

[cmake] print more helpful error messages for builtins-backed options

as suggested by guitargeek
@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 annoying-so but the 'quickly build the ROOT you can on this system for my private usage'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip ci Skip the full builds on the actions runners

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants