Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable automatic 'call home' in cmake when not needed #11603

Closed
wdconinc opened this issue Oct 18, 2022 · 6 comments · Fixed by #16594
Closed

Disable automatic 'call home' in cmake when not needed #11603

wdconinc opened this issue Oct 18, 2022 · 6 comments · Fixed by #16594

Comments

@wdconinc
Copy link
Contributor

Is your feature request related to a problem? Please describe.

  1. We would like to compile ROOT without triggering a 'call home' in the CMakeLists.txt, at https://github.com/root-project/root/blob/master/CMakeLists.txt#L124, so we can install ROOT on network-isolated nodes without incurring artificial build delays, or on network-connected nodes which we would prefer to remain private.
  2. When desired, we would like the ROOT internet connection check to work regardless of captive portals and other web screening portals, since currently those sites may appear as connected because the content of the downloaded file is not checked for accuracy.
  3. We would like to be notified in advance of the privacy policy that governs the data that is (inevitably) collected by the root.cern webservers without the user's consent or knowledge, since there exist user privacy expectations here. GDPR principles that may apply: lack of transparency, collection beyond the limited purpose, lack of data minimization, and probably some others (I am not a laywer).

Describe the solution you'd like

The 'call home' in https://github.com/root-project/root/blob/master/CMakeLists.txt#L124 should default to off, and should only be called when explicitly requested by a user, and only when the features (builtin_gsl and clad) that depend on it are enabled. When it is used, the internet connection check should check a checksum of the downloaded file.

Describe alternatives you've considered

Alternatively, the user can be alerted upon downloading the ROOT source code from any of its various locations that compiling this software may trigger data collection on the root.cern server, e.g. https://root.cern/install/build_from_source/, https://github.com/root-project/root/releases, etc.

@bellenot
Copy link
Member

bellenot commented Oct 24, 2022

So we could introduce such option (e.g check_connectivity), set it ON by default (to keep the current behavior), and set the NO_CONNECTION to TRUE if it is disabled. Would that fit your need?
And maybe @Axel-Naumann can also comment on this

guitargeek added a commit to guitargeek/root that referenced this issue May 9, 2024
The connection check only makes sense for `fail-on-missing=OFF`, where
the result is used to decide whether to download a missing dependency as
a builtin from the internet, or to disable the feature that has the
missing dependency.

With `fail-on-missing=ON`, it doesn't matter because disabling features
is not allowed. Therefore, we can skip the connection check to save some
configuration overhead and just assume we have internet: if a builtin
can't be downloaded there will be a configuration failure either way.

Closes root-project#11603 without introducing an additional flag.
guitargeek added a commit to guitargeek/root that referenced this issue May 9, 2024
The connection check only makes sense for `fail-on-missing=OFF`, where
the result is used to decide whether to download a missing dependency as
a builtin from the internet, or to disable the feature that has the
missing dependency.

With `fail-on-missing=ON`, it doesn't matter because disabling features
is not allowed. Therefore, we can skip the connection check to save some
configuration overhead and just assume we have internet: if a builtin
can't be downloaded there will be a configuration failure either way.

Also, move the connectivity check after `include(RootBuildOptions)`,
such that the `fail-on-missing` and `clad` options are correctly set.
Before, the code that automatically disabled clad when no connection was
found was essentialy dead, because if the user doesn't specify `clad=ON`
explicitly, it is only set to `ON` in the RootBuildOptions macro.

Closes root-project#11603 without introducing an additional flag.
@guitargeek
Copy link
Contributor

guitargeek commented May 9, 2024

Hi @wdconinc and @bellenot! It would be nice if we could fix the issue without the added complexity of an additional flag.

Wouter, you're probably using the fail-on-missing option to build ROOT, right? The flag to make sure that the features don't get quietly disabled at configuration time.

The connectivity check doesn't make sense with fail-on-missing=ON, because a missing dependency will cause a configuration failure either way. More on this in the description of the PR that I opened:

Would that PR fix your issue?

@wdconinc
Copy link
Contributor Author

wdconinc commented May 9, 2024

The fix seems fine (we are indeed using fail-on-missing by default through the spack package).

It is still not entirely clear in advance which features require connectivity, and which don't (or even what to do in advance in order to pre-populate the FetchContent locations). There is also confusion with the names of the features: builtin_ would lead one to think it's provided with the source tree but it isn't, except for builtin_openui5 where it is the opposite.

@guitargeek
Copy link
Contributor

Cool, good to hear that the PR goes in the right direction then!

It is still not entirely clear in advance which features require connectivity

All features of builtins that do require network note this in their description:

I agree that builtin_openui5 should explicitly say that it requires network if OFF.

For the confusing name with builtin_, do you have a suggestion to make this clearer? I don't think there are many options there, we meant of course builtin to the ROOT build, not the source tree 🙂

About the pre-populating of FetchContent locations: I was facing the same problem recently for nix packages. I fixed it in the end by patching the CMake code of ROOT:
NixOS/nixpkgs#308497

@wdconinc
Copy link
Contributor Author

wdconinc commented May 9, 2024

Since you had to patch it in nix, you'll also appreciate the fragility of such an approach. In fact, it will already break with #15467... (substituteInPlace CMakeLists.txt --replace 'if(NO_CONNECTION)' 'if(FALSE)' will fail to match anything)

I don't want to suggest changing well-established config option names like builtin_*, but I think fetching external content should be a default-off option. Is it reasonable to have builtin_openui5, which looks for an openui5 tree in _deps (or wherever FetchContent places it), and a separate fetch_openui5 which enables the fetching itself? That way, we can have builtin_openui5=ON and fetch_openui5=ON for automatic fetching, and builtin_openui5=ON and fetch_openui5=OFF when we provide openui5 ourselves. This could extend to other keywords, potentially allowing the default to be seamless with current defaults.

guitargeek added a commit to guitargeek/root that referenced this issue May 21, 2024
The connection check only makes sense for `fail-on-missing=OFF`, where
the result is used to decide whether to download a missing dependency as
a builtin from the internet, or to disable the feature that has the
missing dependency.

With `fail-on-missing=ON`, it doesn't matter because disabling features
is not allowed. Therefore, we can skip the connection check to save some
configuration overhead and just assume we have internet: if a builtin
can't be downloaded there will be a configuration failure either way.

Closes root-project#11603 without introducing an additional flag.
guitargeek added a commit to guitargeek/root that referenced this issue Oct 2, 2024
This would address two annoyances I have at the same time:

  1. One can now clone both `root` and `clad`, linking the source
     directories together. This makes it much easier to try out changes
     in clad in the context of ROOT.

  2. ROOT can now configure and build without an internet connection,
     even with `Dclad=ON`. This partially addresses root-project#11603.
guitargeek added a commit that referenced this issue Oct 2, 2024
This would address two annoyances I have at the same time:

  1. One can now clone both `root` and `clad`, linking the source
     directories together. This makes it much easier to try out changes
     in clad in the context of ROOT.

  2. ROOT can now configure and build without an internet connection,
     even with `Dclad=ON`. This partially addresses #11603.
@guitargeek
Copy link
Contributor

guitargeek commented Oct 3, 2024

Thanks @wdconinc for these suggestions!

While it would be nice to make the built options a bit more systematic, I think people got used to them as they are. So I would not change them for now, and instead quite literally implement what you requested initially in this issue:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Issues
4 participants