search

root-project / root

The official repository for ROOT: analyzing, storing and visualizing big data, scientifically
https://root.cern
Other
2.72k stars 1.29k forks source link

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

Closed wdconinc closed 1 month ago

wdconinc commented 2 years ago

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 commented 2 years ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 :slightly_smiling_face:

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: https://github.com/NixOS/nixpkgs/pull/308497

wdconinc commented 6 months ago

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 commented 1 month ago

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: