Add rust-lang CI environment check #102428
Conversation
rustbot
added
the
T-bootstrap
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jyn514 (or someone else) soon. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
jyn514
added
A-testsuite
There was a problem hiding this comment.
This is not complete, you'll want to change at least https://github.com/rust-lang/rust/blob/b97f117e34af40b68a3c82aa6e0ec280c049d178/src/bootstrap/config.rs#L1297 as well.
Having looked at the code, I worry this is less reliable than forcing people to unset GITHUB_ACTIONS in their own CI. We don't have a good way to distinguish forks of rust-lang/rust from projects that are building it unchanged.
cc @Mark-Simulacrum, would be interested to hear your thoughts.
src/bootstrap/native.rs
Outdated
| @@ -193,7 +193,7 @@ pub(crate) fn is_ci_llvm_available(config: &crate::config::Config, asserts: bool | |||
| return false; | |||
| } | |||
|
|
|||
| if crate::util::CiEnv::is_ci() { | |||
| if crate::util::CiEnv::is_rust_lang_ci() { | |||
There was a problem hiding this comment.
I don't think this is correct for forks. Say that llvm_sha == head_sha. That means that LLVM was modified in this commit (even if it's being built from a fork). We don't have enough info here to know whether this is a fork of rust-lang/rust, or whether someone's just downloaded the git history in addition to building from source. To be cautious I think we should avoid trying to download LLVM and force a build from source; people can still unset GITHUB_ACTIONS if this behavior is wrong for them.
There was a problem hiding this comment.
I had indeed missed the check in src/bootstrap/config.rs which enforces stage 2 builds in CI environments, it was the most problematic one for me. As is_ci_llvm_available(), I don't mind leaving it untouched, but rather than undefine GITHUB_ACTIONS, I would prefer adding an environment variable that could force-disable rust-lang CI checks. If you do a search for "is_ci()" you'll find a bunch of other helper functions that check for a CI environment for things that are benign in nature, like colorized output, etc.
This comment has been minimized.
This comment has been minimized.
awakecoding
force-pushed
the
rust-lang-ci-check
branch
2 times, most recently
from
ad36a05
to
b0bea87
Compare
awakecoding
force-pushed
the
rust-lang-ci-check
branch
from
b0bea87
to
fb78d96
Compare
|
I think in general probably every case where we do something different depending on GITHUB_ACTIONS being set isn't right (unless its for things like turning off color or so); we should rely on a src/ci/run.sh-set variable for that (or config.toml option). So I'm in general in favor of the direction of this PR, but I would prefer that we go farther and entirely decouple from github actions-only environment variables in rustbuild itself (and if not possible, discuss that on a case by case basis). |
Mark-Simulacrum
added
S-waiting-on-author
|
@Mark-Simulacrum if I understand what you're saying, is that you would rather require that an environment variable like RUST_LANG_CI be defined explicitly to enable specific "official" CI checks separately from detecting and enabling those checks by default in a CI environment based on environment variables like GITHUB_ACTIONS? Let me know what modifications you would like to have made, and I'll modify my pull request |
|
Of course, if you have a fairly good idea of how you would like to address this specific issue and would rather open a separate pull request with the changes, that is fine with me as well. It doesn't need to be fixed by this specific pull request, all I care about is not having to undefine GITHUB_ACTIONS to avoid triggering the official rust-lang CI checks, it's hackish and makes custom CI builds harder |
|
Yes, that sounds right. I don't have a lot of bandwidth to hunt down each case we rely on the GitHub actions set environment variables to make the determination - I think hopefully it's not too hard for you to make a reasonable decision in most of those places and switch them to using something like RUST_LANG_CI. We can iterate from there. |
|
@awakecoding |
|
☔ The latest upstream changes (presumably #105058) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@awakecoding @rustbot label: +S-inactive |
rustbot
added
the
S-inactive
The current Rust bootstrap build scripts detect CI environments to enforce specific build options in the official rust-lang CI environment. The only problem is that it doesn't really distinguish between the official rust-lang CI environment, and just running in any CI environment. For my own Rust distribution built in GitHub Actions, I had to manually unset the GITHUB_ACTIONS environment variable as a temporary workaround. This pull request adds a new CiEnv::is_rust_lang_ci() check meant to detect when the scripts are executed from the official rust-lang CI environment.
To avoid undefining the GITHUB_ACTIONS environment variable, I added a check for the RUST_LANG_CI environment: if it is defined, it will override the result from CiEnv::is_rust_lang_ci(), if it is not defined, CiEnv::is_rust_lang_ci() will return true if it detects a CI environment. This should be sufficient to force-disable rust-lang CI specific checks in third-party CI environments, without affecting the current default behavior.