-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[ci] Remove redundant variables in global CI config #19719
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
Conversation
It is important that our CI config doesn't stray away too much from the default ROOT configuration when you build from source. Like this, we are sure that we actually test more or less what people will get when building ROOT. So I would argue that it's better if we don't set configuration flags explicitly if they are set to what is already the default, to keep builds more consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Since `check_connection=ON` is the default from ROOT builds from source, it is important to test this in the CI. But we don't want to test this in general, because of the constant risk that the probe file on `root.cern` can't be downloaded. So we should test it on a platform that uses builtins that trigger the connection check, and where sporadic build failues annoy us the least. The platforms that use lots of builtins and will therefore trigger the connection check forever are `windows` and `macos`. We don't have many Windows CI configs, so we should not risk one of them. Therefore, I suggest to use some `mac` platform. Let's use `mac-beta`, so the sole config with `check_connection=ON` will not be dropped without replacement in the future.
Test Results 21 files 21 suites 3d 12h 12m 56s ⏱️ For more details on these failures, see this check. Results for commit adce111. |
This breaks the option hashing in our CI script. I'm revert in #19721. To elaborate a bit more, I disagree with the reasons given in the commit message: Our CI's purpose is not to test the default configuration, but to make sure that a stable set of (more or less) carefully chosen platforms and options continues to work for every change we commit. In order to incorporate changes in the configuration, our CI script hashes all options to determine if an incremental build makes sense. By relying on default values that can potentially change, this mechanism is rendered useless. |
We use the same configuration for testing and for building the binaries that we ship to the users, if I understand correctly. If the But if Let me also add that I think it's good that we use the same configuration for CI and releases. because like this we actually test what we intend to release. |
Also, do the internet connection check in one of our CI configurations, as requested by @pcanal (#19632 (comment)).
More (maybe too much) detail in the commit descriptions.