Skip to content

Conversation

guitargeek
Copy link
Contributor

@guitargeek guitargeek commented Aug 21, 2025

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.

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.
@guitargeek guitargeek self-assigned this Aug 21, 2025
@guitargeek guitargeek requested a review from dpiparo as a code owner August 21, 2025 21:55
@guitargeek guitargeek added clean build Ask CI to do non-incremental build on PR in:CI labels Aug 21, 2025
Copy link
Member

@pcanal pcanal left a 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.
Copy link

Test Results

    21 files      21 suites   3d 12h 12m 56s ⏱️
 3 557 tests  3 556 ✅ 0 💤 1 ❌
72 950 runs  72 949 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit adce111.

@guitargeek guitargeek merged commit c84322f into root-project:master Aug 22, 2025
24 of 26 checks passed
@guitargeek guitargeek deleted the ci_config branch August 22, 2025 03:48
@hahnjo
Copy link
Member

hahnjo commented Aug 22, 2025

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.

@guitargeek
Copy link
Contributor Author

guitargeek commented Aug 22, 2025

We use the same configuration for testing and for building the binaries that we ship to the users, if I understand correctly. If the global.txt would only be used for our CI tests and then we have a separate configuration for the releases that only deviates from the defaults in a controlled and justified way, then I would agree with you.

But if globals.txt is used for the binaries, option divergence ultimately leads to user confusion, like "Why does this script from my colleague not work on my PC? He built ROOT from source and I downloaded the binaries". I have had these questions before.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean build Ask CI to do non-incremental build on PR in:CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants