[CMake] Skip connection check if fail-on-missing=ON
#15467
Conversation
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.
The connection check if not done anymore if `fail-on-missing=ON` and `NO_CONNECTION` is always set to false. Now, the configuration will fail at download time, and not with fatal errors in `SearchInstalledSoftware`. Therefore, the corresponding code branches can be removed.
Test Results 10 files 10 suites 1d 21h 21m 36s ⏱️ Results for commit da32e95. |
| # 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. |
There was a problem hiding this comment.
What does the error look like in this case? (Part of the connection test was too make the error 'readable/clear').
There was a problem hiding this comment.
I haven't tried this out, but the error will be some sort of download error when it attempts to download the builtin. Indeed that's a bit less clear, so the question here is really: do nicer error messages justify the connectivity check in the fail-on-missing=ON case? I would argue "no", because fail-on-missing is only used by experts anyway that will be able to also understand the download errors and make a decision whether they prefer to get internet or disable the feature that required this download. Maybe to help them out a little bit, we should mention which feature use a given builtin in the options documentation string for the builtin_ options?
There was a problem hiding this comment.
I haven't tried this out,
We should know rather than guess.
Do nicer error messages justify the connectivity check in the fail-on-missing=ON case?
We might have a way (with more typing) to have both by adding the test (with 'if already check don't check again) in each of the code portion that will trigger a download (actually it might 'as simple as' adding a wrapper around ExternalProject_Add)
There was a problem hiding this comment.
Ah yes, that's a good idea! To "lazily" do the connection check only when a download is attempted 👍 I'll see how we can do this.
There was a problem hiding this comment.
Wait, it's not so easy actually, because some people pre-populate the sources that would be retrieved by FetchContent steered with CMake variable. So the logic to determine if the connection check needs to be done has to consider that too (see the discussion at the end of #11603). So I come back to my original conclusion: we should consider sacrificing the nicer error messages for a less fragile and simpler configuration.
In fact, right now it's already a problem because the connection check is always done and can trigger the FATAL_ERROR code patch in SearchInstalledSoftware, even if the FetchContent is sideloaded.
There was a problem hiding this comment.
Wait, it's not so easy actually, because some people pre-populate the sources that would be retrieved by FetchContent
If we put it in the wrap we should be able to check if the files/directories already exist.
There was a problem hiding this comment.
I don't think this goes in the direction of making things less fragile and complicated, which was one of the considerations for the issue:
#11603 (comment)
In that case, we might as well implement the flag for the connectivity check, as suggested here:
#11603 (comment)
|
Closing the PR because the download errors are less informative than the existing errors conditional on the connectivity check, and they are less practical: they happen at build time instead of configuration time. Since this is a concern, we might as well go for the solution of optionally disabling the connectivity check. I'll spin off the bugfix with the connection check for clad being at the wrong place to a separate PR. |
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 #11603 without introducing an additional flag.
To be backported to 6.32, because it will improve the packaging of the release.