Skip to content
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

extend the "if-unchanged" logic for compiler builds #131831

Merged
merged 7 commits into from
Nov 12, 2024
84 changes: 67 additions & 17 deletions src/bootstrap/src/core/config/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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",
onur-ozkan marked this conversation as resolved.
Show resolved Hide resolved
":!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",
Copy link
Member

@RalfJung RalfJung Nov 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bunch of miscellaneous stuff in src/etc. Are you 100% confident none of it affects the build?

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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So which ones are worth caching? tests, src/tools -- anything else?

Copy link
Member Author

Choose a reason for hiding this comment

The 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).

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build_helper is a dependency of bootstrap, but excluded here, which seems wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's no longer part of src/tools.

":!src/README.md",
];

macro_rules! check_ci_llvm {
($name:expr) => {
assert!(
Expand Down Expand Up @@ -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 =
Expand Down
17 changes: 16 additions & 1 deletion src/bootstrap/src/core/config/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use clap::CommandFactory;
use serde::Deserialize;

use super::flags::Flags;
use super::{ChangeIdWrapper, Config};
use super::{ChangeIdWrapper, Config, RUSTC_IF_UNCHANGED_ALLOWED_PATHS};
use crate::core::build_steps::clippy::get_clippy_rules_in_order;
use crate::core::build_steps::llvm;
use crate::core::config::{LldMode, Target, TargetSelection, TomlConfig};
Expand Down Expand Up @@ -410,3 +410,18 @@ fn jobs_precedence() {
);
assert_eq!(config.jobs, Some(123));
}

#[test]
fn check_rustc_if_unchanged_paths() {
let config = parse("");
let normalised_allowed_paths: Vec<_> = RUSTC_IF_UNCHANGED_ALLOWED_PATHS
.iter()
.map(|t| {
t.strip_prefix(":!").expect(&format!("{t} doesn't have ':!' prefix, but it should."))
})
.collect();

for p in normalised_allowed_paths {
assert!(config.src.join(p).exists(), "{p} doesn't exist.");
}
}
Loading