From 71fd3abc73471bf1b68666c980d73c4385e5e9a7 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 26 Nov 2022 15:00:39 -0500 Subject: [PATCH 1/3] Don't update submodules for `x setup` Before, the submodule handling was very jank and would update *between two interactive prompts*: ``` ; x setup Building rustbuild Finished dev [unoptimized] target(s) in 0.05s Welcome to the Rust project! What do you want to do with x.py? a) library: Contribute to the standard library Please choose one (a/b/c/d/e): a Updating submodule library/backtrace Submodule 'library/backtrace' (https://github.com/rust-lang/backtrace-rs.git) registered for path 'library/backtrace' error: you asked `x.py` to setup a new config file, but one already exists at `config.toml` Build completed unsuccessfully in 0:00:02 ``` That's not a great user experience because you need to wait a long time between prompts. It would be possible to move the submodule handling either before or after the prompt, but it seems better to just not require submodules to be checked out at all, to minimize the time spend waiting just to create a new configuration. --- src/bootstrap/flags.rs | 9 +++++---- src/bootstrap/lib.rs | 44 ++++++++++++++++++++++++------------------ src/bootstrap/setup.rs | 3 ++- 3 files changed, 32 insertions(+), 24 deletions(-) diff --git a/src/bootstrap/flags.rs b/src/bootstrap/flags.rs index 2001e29bd2ead..37a8eb884efb0 100644 --- a/src/bootstrap/flags.rs +++ b/src/bootstrap/flags.rs @@ -143,7 +143,7 @@ pub enum Subcommand { args: Vec, }, Setup { - profile: Profile, + profile: Option, }, } @@ -628,14 +628,15 @@ Arguments: |path| format!("{} is not a valid UTF8 string", path.to_string_lossy()) )); - profile_string.parse().unwrap_or_else(|err| { + let profile = profile_string.parse().unwrap_or_else(|err| { eprintln!("error: {}", err); eprintln!("help: the available profiles are:"); eprint!("{}", Profile::all_for_help("- ")); crate::detail_exit(1); - }) + }); + Some(profile) } else { - t!(crate::setup::interactive_path()) + None }; Subcommand::Setup { profile } } diff --git a/src/bootstrap/lib.rs b/src/bootstrap/lib.rs index f4fa556b97450..60ce431cb0c39 100644 --- a/src/bootstrap/lib.rs +++ b/src/bootstrap/lib.rs @@ -542,16 +542,6 @@ impl Build { metrics: metrics::BuildMetrics::init(), }; - build.verbose("finding compilers"); - cc_detect::find(&mut build); - // When running `setup`, the profile is about to change, so any requirements we have now may - // be different on the next invocation. Don't check for them until the next time x.py is - // run. This is ok because `setup` never runs any build commands, so it won't fail if commands are missing. - if !matches!(build.config.cmd, Subcommand::Setup { .. }) { - build.verbose("running sanity check"); - sanity::check(&mut build); - } - // If local-rust is the same major.minor as the current version, then force a // local-rebuild let local_version_verbose = @@ -567,16 +557,32 @@ impl Build { build.local_rebuild = true; } - // Make sure we update these before gathering metadata so we don't get an error about missing - // Cargo.toml files. - let rust_submodules = - ["src/tools/rust-installer", "src/tools/cargo", "library/backtrace", "library/stdarch"]; - for s in rust_submodules { - build.update_submodule(Path::new(s)); - } + build.verbose("finding compilers"); + cc_detect::find(&mut build); + // When running `setup`, the profile is about to change, so any requirements we have now may + // be different on the next invocation. Don't check for them until the next time x.py is + // run. This is ok because `setup` never runs any build commands, so it won't fail if commands are missing. + // + // Similarly, for `setup` we don't actually need submodules or cargo metadata. + if !matches!(build.config.cmd, Subcommand::Setup { .. }) { + build.verbose("running sanity check"); + sanity::check(&mut build); - build.verbose("learning about cargo"); - metadata::build(&mut build); + // Make sure we update these before gathering metadata so we don't get an error about missing + // Cargo.toml files. + let rust_submodules = [ + "src/tools/rust-installer", + "src/tools/cargo", + "library/backtrace", + "library/stdarch", + ]; + for s in rust_submodules { + build.update_submodule(Path::new(s)); + } + + build.verbose("learning about cargo"); + metadata::build(&mut build); + } build } diff --git a/src/bootstrap/setup.rs b/src/bootstrap/setup.rs index 04480277fe047..2507b7a29b06f 100644 --- a/src/bootstrap/setup.rs +++ b/src/bootstrap/setup.rs @@ -81,8 +81,9 @@ impl fmt::Display for Profile { } } -pub fn setup(config: &Config, profile: Profile) { +pub fn setup(config: &Config, profile: Option) { let path = &config.config.clone().unwrap_or(PathBuf::from("config.toml")); + let profile = profile.unwrap_or_else(|| t!(interactive_path())); if path.exists() { eprintln!( From 86251dabac6252ebd3c2a90bd4695563d202b919 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 26 Nov 2022 15:08:41 -0500 Subject: [PATCH 2/3] Refactor `setup_config_toml` into a function --- src/bootstrap/setup.rs | 62 +++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/src/bootstrap/setup.rs b/src/bootstrap/setup.rs index 2507b7a29b06f..bad6accc78472 100644 --- a/src/bootstrap/setup.rs +++ b/src/bootstrap/setup.rs @@ -1,15 +1,13 @@ +use crate::Config; use crate::{t, VERSION}; -use crate::{Config, TargetSelection}; use std::env::consts::EXE_SUFFIX; use std::fmt::Write as _; use std::fs::File; +use std::io::Write; use std::path::{Path, PathBuf, MAIN_SEPARATOR}; use std::process::Command; use std::str::FromStr; -use std::{ - env, fmt, fs, - io::{self, Write}, -}; +use std::{fmt, fs, io}; #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum Profile { @@ -84,34 +82,10 @@ impl fmt::Display for Profile { pub fn setup(config: &Config, profile: Option) { let path = &config.config.clone().unwrap_or(PathBuf::from("config.toml")); let profile = profile.unwrap_or_else(|| t!(interactive_path())); + setup_config_toml(path, profile, config); - if path.exists() { - eprintln!( - "error: you asked `x.py` to setup a new config file, but one already exists at `{}`", - path.display() - ); - eprintln!("help: try adding `profile = \"{}\"` at the top of {}", profile, path.display()); - eprintln!( - "note: this will use the configuration in {}", - profile.include_path(&config.src).display() - ); - crate::detail_exit(1); - } - - let settings = format!( - "# Includes one of the default files in src/bootstrap/defaults\n\ - profile = \"{}\"\n\ - changelog-seen = {}\n", - profile, VERSION - ); - t!(fs::write(path, settings)); - - let include_path = profile.include_path(&config.src); - println!("`x.py` will now use the configuration at {}", include_path.display()); - - let build = TargetSelection::from_user(&env!("BUILD_TRIPLE")); let stage_path = - ["build", build.rustc_target_arg(), "stage1"].join(&MAIN_SEPARATOR.to_string()); + ["build", config.build.rustc_target_arg(), "stage1"].join(&MAIN_SEPARATOR.to_string()); println!(); @@ -153,6 +127,32 @@ pub fn setup(config: &Config, profile: Option) { } } +fn setup_config_toml(path: &PathBuf, profile: Profile, config: &Config) { + if path.exists() { + eprintln!( + "error: you asked `x.py` to setup a new config file, but one already exists at `{}`", + path.display() + ); + eprintln!("help: try adding `profile = \"{}\"` at the top of {}", profile, path.display()); + eprintln!( + "note: this will use the configuration in {}", + profile.include_path(&config.src).display() + ); + crate::detail_exit(1); + } + + let settings = format!( + "# Includes one of the default files in src/bootstrap/defaults\n\ + profile = \"{}\"\n\ + changelog-seen = {}\n", + profile, VERSION + ); + t!(fs::write(path, settings)); + + let include_path = profile.include_path(&config.src); + println!("`x.py` will now use the configuration at {}", include_path.display()); +} + fn rustup_installed() -> bool { Command::new("rustup") .arg("--version") From b771d901f735aa6d4078aed92b03e30eba35bf5a Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 26 Nov 2022 15:43:10 -0500 Subject: [PATCH 3/3] Revamp the order `setup` executes - Create `config.toml` last. It's the most likely to error, and used to stop later steps from executing - Don't print an error message + exit if the git hook already exists; that's expected --- src/bootstrap/setup.rs | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/bootstrap/setup.rs b/src/bootstrap/setup.rs index bad6accc78472..c7f98a7d0d149 100644 --- a/src/bootstrap/setup.rs +++ b/src/bootstrap/setup.rs @@ -80,15 +80,10 @@ impl fmt::Display for Profile { } pub fn setup(config: &Config, profile: Option) { - let path = &config.config.clone().unwrap_or(PathBuf::from("config.toml")); let profile = profile.unwrap_or_else(|| t!(interactive_path())); - setup_config_toml(path, profile, config); - let stage_path = ["build", config.build.rustc_target_arg(), "stage1"].join(&MAIN_SEPARATOR.to_string()); - println!(); - if !rustup_installed() && profile != Profile::User { eprintln!("`rustup` is not installed; cannot link `stage1` toolchain"); } else if stage_dir_exists(&stage_path[..]) { @@ -109,8 +104,6 @@ pub fn setup(config: &Config, profile: Option) { Profile::User => &["dist", "build"], }; - println!(); - t!(install_git_hook_maybe(&config)); println!(); @@ -125,10 +118,14 @@ pub fn setup(config: &Config, profile: Option) { "For more suggestions, see https://rustc-dev-guide.rust-lang.org/building/suggested.html" ); } + + let path = &config.config.clone().unwrap_or(PathBuf::from("config.toml")); + setup_config_toml(path, profile, config); } fn setup_config_toml(path: &PathBuf, profile: Profile, config: &Config) { if path.exists() { + eprintln!(); eprintln!( "error: you asked `x.py` to setup a new config file, but one already exists at `{}`", path.display() @@ -304,7 +301,18 @@ pub fn interactive_path() -> io::Result { // install a git hook to automatically run tidy --bless, if they want fn install_git_hook_maybe(config: &Config) -> io::Result<()> { + let git = t!(config.git().args(&["rev-parse", "--git-common-dir"]).output().map(|output| { + assert!(output.status.success(), "failed to run `git`"); + PathBuf::from(t!(String::from_utf8(output.stdout)).trim()) + })); + let dst = git.join("hooks").join("pre-push"); + if dst.exists() { + // The git hook has already been set up, or the user already has a custom hook. + return Ok(()); + } + let mut input = String::new(); + println!(); println!( "Rust's CI will automatically fail if it doesn't pass `tidy`, the internal tool for ensuring code quality. If you'd like, x.py can install a git hook for you that will automatically run `tidy --bless` before @@ -330,12 +338,6 @@ undesirable, simply delete the `pre-push` file from .git/hooks." if should_install { let src = config.src.join("src").join("etc").join("pre-push.sh"); - let git = - t!(config.git().args(&["rev-parse", "--git-common-dir"]).output().map(|output| { - assert!(output.status.success(), "failed to run `git`"); - PathBuf::from(t!(String::from_utf8(output.stdout)).trim()) - })); - let dst = git.join("hooks").join("pre-push"); match fs::hard_link(src, &dst) { Err(e) => eprintln!( "error: could not create hook {}: do you already have the git hook installed?\n{}",