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

Add a new download-ci-llvm = if-unchanged option and enable it by default for profile = codegen #116881

Merged
merged 1 commit into from
Nov 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions config.example.toml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
#
# If `change-id` does not match the version that is currently running,
# `x.py` will prompt you to update it and check the related PR for more details.
change-id = 117435
change-id = 116881

# =============================================================================
# Tweaking how LLVM is compiled
Expand All @@ -42,11 +42,15 @@ change-id = 117435
# Unless you're developing for a target where Rust CI doesn't build a compiler
# toolchain or changing LLVM locally, you probably want to leave this enabled.
#
# All tier 1 targets are currently supported; set this to `"if-available"` if
# you are not sure whether you're on a tier 1 target.
# Set this to `"if-available"` if you are not sure whether you're on a tier 1
# target. All tier 1 targets are currently supported;
#
# We also currently only support this when building LLVM for the build triple.
#
# Set this to `"if-unchanged"` to only download if the llvm-project have not
# been modified. (If there are no changes or if built from tarball source,
# the logic is the same as "if-available")
#
# Note that many of the LLVM options are not currently supported for
# downloading. Currently only the "assertions" option can be toggled.
#download-ci-llvm = if rust.channel == "dev" { "if-available" } else { false }
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/defaults/config.codegen.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ assertions = true
# enable warnings during the llvm compilation
enable-warnings = true
# build llvm from source
download-ci-llvm = false
download-ci-llvm = "if-unchanged"

[rust]
# This enables `RUSTC_LOG=debug`, avoiding confusing situations
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/download-ci-llvm-stamp
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
Change this file to make users of the `download-ci-llvm` configuration download
a new version of LLVM from CI, even if the LLVM submodule hasn’t changed.

Last change is for: https://github.com/rust-lang/rust/pull/113996
Last change is for: https://github.com/rust-lang/rust/pull/116881
26 changes: 11 additions & 15 deletions src/bootstrap/src/core/build_steps/dist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2219,23 +2219,19 @@ impl Step for RustDev {
builder.ensure(crate::core::build_steps::llvm::Lld { target });

let src_bindir = builder.llvm_out(target).join("bin");
// If updating this list, you likely want to change
// If updating this, you likely want to change
// src/bootstrap/download-ci-llvm-stamp as well, otherwise local users
// will not pick up the extra file until LLVM gets bumped.
for bin in &[
"llvm-config",
"llvm-ar",
"llvm-objdump",
"llvm-profdata",
"llvm-bcanalyzer",
"llvm-cov",
"llvm-dwp",
"llvm-nm",
"llvm-dwarfdump",
"llvm-dis",
"llvm-tblgen",
] {
tarball.add_file(src_bindir.join(exe(bin, target)), "bin", 0o755);
// We should include all the build artifacts obtained from a source build,
// so that you can use the downloadable LLVM as if you’ve just run a full source build.
if src_bindir.exists() {
for entry in walkdir::WalkDir::new(&src_bindir) {
let entry = t!(entry);
if entry.file_type().is_file() && !entry.path_is_symlink() {
let name = entry.file_name().to_str().unwrap();
tarball.add_file(src_bindir.join(name), "bin", 0o755);
}
}
}

// We don't build LLD on some platforms, so only add it if it exists
Expand Down
9 changes: 9 additions & 0 deletions src/bootstrap/src/core/build_steps/setup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,15 @@ impl Step for Profile {
}

fn run(self, builder: &Builder<'_>) {
// During ./x.py setup once you select the codegen profile.
// The submodule will be downloaded. It does not work in the
// tarball case since they don't include Git and submodules
// are already included.
if !builder.rust_info().is_from_tarball() {
if self == Profile::Codegen {
builder.update_submodule(&Path::new("src/llvm-project"));
}
}
setup(&builder.build.config, self)
}
}
Expand Down
101 changes: 90 additions & 11 deletions src/bootstrap/src/core/config/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use std::process::Command;
use std::str::FromStr;

use crate::core::build_steps::compile::CODEGEN_BACKEND_PREFIX;
use crate::core::build_steps::llvm;
use crate::core::config::flags::{Color, Flags, Warnings};
use crate::utils::cache::{Interned, INTERNER};
use crate::utils::channel::{self, GitInfo};
Expand Down Expand Up @@ -1530,17 +1531,7 @@ impl Config {
config.llvm_build_config = llvm.build_config.clone().unwrap_or(Default::default());

let asserts = llvm_assertions.unwrap_or(false);
config.llvm_from_ci = match llvm.download_ci_llvm {
Some(StringOrBool::String(s)) => {
assert_eq!(s, "if-available", "unknown option `{s}` for download-ci-llvm");
crate::core::build_steps::llvm::is_ci_llvm_available(&config, asserts)
}
Some(StringOrBool::Bool(b)) => b,
None => {
config.channel == "dev"
&& crate::core::build_steps::llvm::is_ci_llvm_available(&config, asserts)
}
};
config.llvm_from_ci = config.parse_download_ci_llvm(llvm.download_ci_llvm, asserts);

if config.llvm_from_ci {
// None of the LLVM options, except assertions, are supported
Expand Down Expand Up @@ -2104,6 +2095,94 @@ impl Config {

Some(commit.to_string())
}

fn parse_download_ci_llvm(
&self,
download_ci_llvm: Option<StringOrBool>,
asserts: bool,
) -> bool {
match download_ci_llvm {
None => self.channel == "dev" && llvm::is_ci_llvm_available(&self, asserts),
Some(StringOrBool::Bool(b)) => b,
Some(StringOrBool::String(s)) if s == "if-available" => {
llvm::is_ci_llvm_available(&self, asserts)
}
Some(StringOrBool::String(s)) if s == "if-unchanged" => {
// Git is needed to track modifications here, but tarball source is not available.
// If not modified here or built through tarball source, we maintain consistency
// with '"if available"'.
if !self.rust_info.is_from_tarball()
&& self
.last_modified_commit(&["src/llvm-project"], "download-ci-llvm", true)
.is_none()
{
// there are some untracked changes in the the given paths.
false
} else {
llvm::is_ci_llvm_available(&self, asserts)
}
}
Some(StringOrBool::String(other)) => {
panic!("unrecognized option for download-ci-llvm: {:?}", other)
}
}
}

/// Returns the last commit in which any of `modified_paths` were changed,
/// or `None` if there are untracked changes in the working directory and `if_unchanged` is true.
pub fn last_modified_commit(
&self,
modified_paths: &[&str],
option_name: &str,
if_unchanged: bool,
) -> Option<String> {
// Handle running from a directory other than the top level
let top_level = output(self.git().args(&["rev-parse", "--show-toplevel"]));
let top_level = top_level.trim_end();

// Look for a version to compare to based on the current commit.
// Only commits merged by bors will have CI artifacts.
let merge_base = output(
self.git()
.arg("rev-list")
.arg(format!("--author={}", self.stage0_metadata.config.git_merge_commit_email))
.args(&["-n1", "--first-parent", "HEAD"]),
);
let commit = merge_base.trim_end();
if commit.is_empty() {
println!("error: could not find commit hash for downloading components from CI");
println!("help: maybe your repository history is too shallow?");
println!("help: consider disabling `{option_name}`");
println!("help: or fetch enough history to include one upstream commit");
crate::exit!(1);
}

// Warn if there were changes to the compiler or standard library since the ancestor commit.
let mut git = self.git();
git.args(&["diff-index", "--quiet", &commit, "--"]);

for path in modified_paths {
git.arg(format!("{top_level}/{path}"));
}

let has_changes = !t!(git.status()).success();
if has_changes {
if if_unchanged {
if self.verbose > 0 {
println!(
"warning: saw changes to one of {modified_paths:?} since {commit}; \
ignoring `{option_name}`"
);
}
return None;
}
println!(
"warning: `{option_name}` is enabled, but there are changes to one of {modified_paths:?}"
);
}

Some(commit.to_string())
}
}

fn set<T>(field: &mut T, val: Option<T>) {
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ const LLD_FILE_NAMES: &[&str] = &["ld.lld", "ld64.lld", "lld-link", "wasm-ld"];
///
/// If you make any major changes (such as adding new values or changing default values), please
/// ensure that the associated PR ID is added to the end of this list.
pub const CONFIG_CHANGE_HISTORY: &[usize] = &[115898, 116998, 117435];
pub const CONFIG_CHANGE_HISTORY: &[usize] = &[115898, 116998, 117435, 116881];
Copy link
Member

Choose a reason for hiding this comment

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

I'm getting a WARNING pointing me to this PR. Do I need to do something? Or can I keep my existing config? It's unclear to me. The PR description says there's a new config, but why does everyone have to be alerted of its existence?

Copy link
Member

Choose a reason for hiding this comment

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

Also this list is unordered now, that seems wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update change-id = 116881 instead may be help.
AFAIK This is ordered by merge time to keep the source code synchronized with the config file.

Copy link
Member

Choose a reason for hiding this comment

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

update change-id = 116881 instead may be help.

That will silence the warning, sure. But the reason the warning is shown must be that I should do something before silencing it. There's no point in having people mindlessly increment a number every week just to get rid of the latest warning.

Copy link
Member

Choose a reason for hiding this comment

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

Also this list is unordered now, that seems wrong?

/// If you make any major changes (such as adding new values or changing default values),
/// please ensure that the associated PR ID is added to the end of this list.
/// This is necessary because the list must be sorted by the merge date.

I'm getting a WARNING pointing me to this PR. Do I need to do something? Or can I keep my existing config? It's unclear to me

# build llvm from source
download-ci-llvm = "if-unchanged"

PR updated the default value here. I am pretty sure we have people who wants to know this change.

Copy link
Member

Choose a reason for hiding this comment

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

Then the PR description needs to say: "if you are coming here from x.py, here's what do do: if you are using the 'codegen' profile, be aware that the default value changed. otherwise, you can ignore this".

Currently I'm coming here clueless and wasting 5min trying to figure out if I should do something, before giving up, frustrated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's my bad, I will add the description for this PR later


/// Extra --check-cfg to add when building
/// (Mode restriction, config name, config values (if any))
Expand Down
Loading