Respect the --ci flag in more places in bootstrap

[Shunpoco]

Motivation

Currently, the way of checking CI environment in bootstrap is not unified. Sometimes --ci flag is respected, but in other cases only GITHUB_ACTIONS env var is checked.

Change

This PR modifies the way of treating the flag in bootstrap. If --ci (or --ci=true) is added, bootstrap's config set ci_env as CiEnv::GithubActions. This PR also modifies some lines in bootstrap to respect the config's CiEnv instance.

Note

I haven't touched anything in tidy, because I want to raise another PR for introducing --ci flag in tidy. In the PR, I will need to change how tidy treats CI information. So currently I leave it as it is.

[Kobzol]

--ci is a bootstrap thing, while CiEnv is in build_helper, to be shared with multiple tools (which might have their own CLI flags or CI semantics). Let's not add any flag logic to build_helper, please.

Replacing uses of CiEnv with reading config.is_running_on_ci in bootstrap is good though.

[Shunpoco]

Thanks, now I understand that bootstrap's --ci flag should not integrated into CiEnv.

But I still think it's worth to introduce Other variant into CiEnv. In order to make a tool (CiEnv user) behave closer to actual CI environment with a ci flag, CiEnv should say "this running environment is some kinds of CI env", because some functions (like get_closest_upstream_commit in src/build_helper/src/git.rs) takes a CiEnv instance as an argument, and I don't think passing CiEnv::GitHubActions when --ci=true is a good idea.
I know that my CiEnv::new(flag) is not a good function since it's tied with Bootstrap's flag. I can move the function into bootstrap's config. (So a tool will have its own constructor function for CiEnv if it has --ci flag).

What do you think about it?

[Kobzol]

I don't think passing CiEnv::GitHubActions when --ci=true is a good idea.

Why? That's exactly what should happen, if we override the mode to --ci=true, the code should assume it is running under CI.

[Shunpoco]

Because CiEnv has None/GithubActions variants, not None/CI variants (nor just a boolean). I was expecting that in the future we would have a new variant for another CI env (I don't think migrating CI is easy task, though). So I thought the separation between GithubActions env and Other env (in this PR, from ci flag) was meaningful.

But if you think all CI env should go to CiEnv::GithubActions, I think that's fine. your way is much simpler than what I thought (now I feel that I was thinking too much).

So can I write such a code in bootstrap config's parse_inner()?

        let ci_env = match flags_ci {
            Some(true) => CiEnv::GitHubActions,
            Some(false) => CiEnv::None,
            None => CiEnv::current(),
        };

[Kobzol]

I was expecting that in the future we would have a new variant for another CI env

It's actually the other way around - we used to have more variants there (e.g. Azure pipelines), but those were removed as they stopped being used in our CI. We don't really explicitly support any downstream usage of our CI, so we only maintain what we use upstream, and that is currently just GitHub Actions. We could turn --ci into a string option if we ever have more CI providers in the future, but for now a boolean is enough. This option is useful mostly only for bootstrap developers and bootstrap tests anyway.

So can I write such a code in bootstrap config's parse_inner()?

Yeah, sounds reasonable, because we anyway have to pass CiEnv to the git build_helper functions, so we might as well centralize its creation, rather than CiEnv::current() ad-hoc. That sounds like an improvement :)

[Kobzol]

r? kobzol

[Shunpoco]

Thanks for the clarification! Now I fully understand that.

[Kobzol]

Default is a bit of a footgun here, and it's unclear to me that not being on CI is a good default anyway. ::current() should be used instead.

Since default seems to be unused, I think that you can just revert this change?

[Shunpoco]

I added Default to CiEnv because bootstrap's Config has Default (now ci_env is a member of Config).
But I just find that no one use Config::default(), so I will drop Default from both Config and CiEnv.

Update: I dropped Defaults in 7e22e90

[Kobzol]

Thank you, left one nit. After fixing it, you can r=me.

@bors delegate

[rust-bors]

✌️ @Shunpoco, you can now approve this pull request!

If @Kobzol told you to "r=me" after making some further change, then please make that change and post @bors r=Kobzol.

[Shunpoco]

Thanks for the review! Now I squashed my commits into one.

@bors r=Kobzol

[rust-bors]

📌 Commit d5574c5 has been approved by Kobzol

It is now in the queue for this repository.

[Kobzol]

Thanks! The Default impl on Config is a legacy leftover, it shouldn't ever be used, so that's a nice cleanup on its own.