-
Notifications
You must be signed in to change notification settings - Fork 13.4k
extend the "if-unchanged" logic for compiler builds #131831
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
Changes from 3 commits
baa95ef
72e63e3
a00fd7a
6b38a15
bc75310
4dfc052
2d143ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,55 @@ use crate::utils::cache::{INTERNER, Interned}; | |
use crate::utils::channel::{self, GitInfo}; | ||
use crate::utils::helpers::{self, exe, output, t}; | ||
|
||
/// Each path in this list is considered "allowed" in the `download-rustc="if-unchanged"` logic. | ||
/// This means they can be modified and changes to these paths should never trigger a compiler build | ||
/// when "if-unchanged" is set. | ||
/// | ||
/// NOTE: Paths must have the ":!" prefix to tell git to ignore changes in those paths during | ||
/// the diff check. | ||
/// | ||
/// WARNING: Be cautious when adding paths to this list. If a path that influences the compiler build | ||
/// is added here, it will cause bootstrap to skip necessary rebuilds, which may lead to risky results. | ||
/// For example, "src/bootstrap" should never be included in this list as it plays a crucial role in the | ||
/// final output/compiler, which can be significantly affected by changes made to the bootstrap sources. | ||
pub(crate) const RUSTC_IF_UNCHANGED_ALLOWED_PATHS: &[&str] = &[ | ||
":!.clang-format", | ||
":!.editorconfig", | ||
":!.git-blame-ignore-revs", | ||
":!.gitattributes", | ||
":!.gitignore", | ||
":!.gitmodules", | ||
":!.ignore", | ||
":!.mailmap", | ||
":!CODE_OF_CONDUCT.md", | ||
":!CONTRIBUTING.md", | ||
":!COPYRIGHT", | ||
":!INSTALL.md", | ||
":!LICENSE-APACHE", | ||
":!LICENSE-MIT", | ||
":!LICENSES", | ||
":!README.md", | ||
":!RELEASES.md", | ||
":!REUSE.toml", | ||
":!config.example.toml", | ||
":!configure", | ||
":!rust-bors.toml", | ||
":!rustfmt.toml", | ||
":!tests", | ||
":!triagebot.toml", | ||
":!x", | ||
":!x.ps1", | ||
":!x.py", | ||
onur-ozkan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
":!src/ci/cpu-usage-over-time.py", | ||
":!src/ci/publish_toolstate.sh", | ||
":!src/doc", | ||
":!src/etc", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a bunch of miscellaneous stuff in As others said before me, the attitude here should be to start with an empty list, and only add things where we are fully confident they are fine to add, and adding them is worth it in terms of caching. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it’s too risky to include that and definitely not worth caching. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So which ones are worth caching? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which one doesn’t seem obviously safe to cache from the current list? Because even if they are rarely merged alone with bors, they will still be very useful in PR CI (by cutting one of the runner’s times in half). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As several people said above, that is the wrong question. You had several items in your list that turned out to be potentially problematic. Clearly you considered them "obviously safe", and they weren't. So unless there's some obvious benefit to including a directory -- like, we regularly have PRs that change only those files -- it should not be included. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually I never considered them "obviously safe". I wasn’t sure about them and was hoping to get some help from the reviews. But sure, I can reduce the list even more. |
||
":!src/librustdoc", | ||
":!src/rustdoc-json-types", | ||
":!src/tools", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's no longer part of |
||
":!src/README.md", | ||
]; | ||
|
||
macro_rules! check_ci_llvm { | ||
($name:expr) => { | ||
assert!( | ||
|
@@ -2768,32 +2817,33 @@ impl Config { | |
} | ||
}; | ||
|
||
let mut files_to_track = vec!["compiler", "src/version", "src/stage0", "src/ci/channel"]; | ||
// RUSTC_IF_UNCHANGED_ALLOWED_PATHS | ||
let mut allowed_paths = RUSTC_IF_UNCHANGED_ALLOWED_PATHS.to_vec(); | ||
|
||
// In CI, disable ci-rustc if there are changes in the library tree. But for non-CI, ignore | ||
// In CI, disable ci-rustc if there are changes in the library tree. But for non-CI, allow | ||
// these changes to speed up the build process for library developers. This provides consistent | ||
// functionality for library developers between `download-rustc=true` and `download-rustc="if-unchanged"` | ||
// options. | ||
if CiEnv::is_ci() { | ||
files_to_track.push("library"); | ||
if !CiEnv::is_ci() { | ||
allowed_paths.push(":!library"); | ||
} | ||
|
||
// Look for a version to compare to based on the current commit. | ||
// Only commits merged by bors will have CI artifacts. | ||
let commit = | ||
match self.last_modified_commit(&files_to_track, "download-rustc", if_unchanged) { | ||
Some(commit) => commit, | ||
None => { | ||
if if_unchanged { | ||
return None; | ||
} | ||
println!("ERROR: could not find commit hash for downloading rustc"); | ||
println!("HELP: maybe your repository history is too shallow?"); | ||
println!("HELP: consider disabling `download-rustc`"); | ||
println!("HELP: or fetch enough history to include one upstream commit"); | ||
crate::exit!(1); | ||
let commit = match self.last_modified_commit(&allowed_paths, "download-rustc", if_unchanged) | ||
{ | ||
Some(commit) => commit, | ||
None => { | ||
if if_unchanged { | ||
return None; | ||
} | ||
}; | ||
println!("ERROR: could not find commit hash for downloading rustc"); | ||
println!("HELP: maybe your repository history is too shallow?"); | ||
println!("HELP: consider setting `rust.download-rustc=false` in config.toml"); | ||
println!("HELP: or fetch enough history to include one upstream commit"); | ||
crate::exit!(1); | ||
} | ||
}; | ||
|
||
if CiEnv::is_ci() && { | ||
let head_sha = | ||
|
Uh oh!
There was an error while loading. Please reload this page.