From 04fa2f2a3bab064eb4a71a73be579b5672f26d51 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Fri, 16 Apr 2021 11:48:44 -0400 Subject: [PATCH 1/5] Don't give a hard error on RUSTC_BOOTSTRAP=crate_name --- src/cargo/core/compiler/custom_build.rs | 10 +++++++++- tests/testsuite/build_script_env.rs | 12 ++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs index a7a5ca1ba92..4f8e92cccad 100644 --- a/src/cargo/core/compiler/custom_build.rs +++ b/src/cargo/core/compiler/custom_build.rs @@ -589,7 +589,15 @@ impl BuildOutput { // to set RUSTC_BOOTSTRAP. // If this is a nightly build, setting RUSTC_BOOTSTRAP wouldn't affect the // behavior, so still only give a warning. - if nightly_features_allowed { + // NOTE: cargo only allows nightly features on RUSTC_BOOTSTRAP=1, but we + // want setting any value of RUSTC_BOOTSTRAP to downgrade this to a warning + // (so that `RUSTC_BOOTSTRAP=pkg_name` will work) + let rustc_bootstrap_allows = |name: &str| { + std::env::var("RUSTC_BOOTSTRAP").map_or(false, |var| { + var.split(',').any(|s| s == name) + }) + }; + if nightly_features_allowed || rustc_bootstrap_allows(&*pkg_name) { warnings.push(format!("Cannot set `RUSTC_BOOTSTRAP={}` from {}.\n\ note: Crates cannot set `RUSTC_BOOTSTRAP` themselves, as doing so would subvert the stability guarantees of Rust for your project.", val, whence diff --git a/tests/testsuite/build_script_env.rs b/tests/testsuite/build_script_env.rs index f03a3024a5a..01dc6452ee4 100644 --- a/tests/testsuite/build_script_env.rs +++ b/tests/testsuite/build_script_env.rs @@ -129,4 +129,16 @@ fn rustc_bootstrap() { .masquerade_as_nightly_cargo() .with_stderr_contains("warning: Cannot set `RUSTC_BOOTSTRAP=1` [..]") .run(); + // RUSTC_BOOTSTRAP set to the name of the crate + p.cargo("build") + .env("RUSTC_BOOTSTRAP", "foo") + .with_stderr_contains("warning: Cannot set `RUSTC_BOOTSTRAP=1` [..]") + .run(); + // RUSTC_BOOTSTRAP set to some random value + p.cargo("build") + .env("RUSTC_BOOTSTRAP", "bar") + .with_stderr_contains("error: Cannot set `RUSTC_BOOTSTRAP=1` [..]") + .with_stderr_contains("help: [..] set the environment variable `RUSTC_BOOTSTRAP=foo` [..]") + .with_status(101) + .run(); } From 6d6f4663512c9b9d91e6beb6a6cc617efe3f8e5e Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Fri, 16 Apr 2021 12:24:20 -0400 Subject: [PATCH 2/5] Use the crate name instead of the package name This doesn't work because it uses the name of the build script, which is always build_script_build. I'm not sure what to change it to - the name of the library crate could be different than the name of the package, and there could be multiple different crates being compiled in the same package. --- src/cargo/core/compiler/custom_build.rs | 34 ++++++++++++------------- tests/testsuite/build_script_env.rs | 12 ++++++--- 2 files changed, 26 insertions(+), 20 deletions(-) diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs index 4f8e92cccad..4004c000f5d 100644 --- a/src/cargo/core/compiler/custom_build.rs +++ b/src/cargo/core/compiler/custom_build.rs @@ -4,7 +4,6 @@ use crate::core::compiler::context::Metadata; use crate::core::compiler::job_queue::JobState; use crate::core::{profiles::ProfileRoot, PackageId}; use crate::util::errors::CargoResult; -use crate::util::interning::InternedString; use crate::util::machine_message::{self, Message}; use crate::util::{internal, profile}; use anyhow::Context as _; @@ -270,7 +269,7 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { } }) .collect::>(); - let pkg_name = unit.pkg.name(); + let crate_name = unit.target.crate_name(); let pkg_descr = unit.pkg.to_string(); let build_script_outputs = Arc::clone(&cx.build_script_outputs); let id = unit.pkg.package_id(); @@ -280,7 +279,7 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { let host_target_root = cx.files().host_dest().to_path_buf(); let all = ( id, - pkg_name, + crate_name.clone(), pkg_descr.clone(), Arc::clone(&build_script_outputs), output_file.clone(), @@ -400,7 +399,7 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { paths::write(&root_output_file, paths::path2bytes(&script_out_dir)?)?; let parsed_output = BuildOutput::parse( &output.stdout, - pkg_name, + crate_name, &pkg_descr, &script_out_dir, &script_out_dir, @@ -422,12 +421,12 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { // itself to run when we actually end up just discarding what we calculated // above. let fresh = Work::new(move |state| { - let (id, pkg_name, pkg_descr, build_script_outputs, output_file, script_out_dir) = all; + let (id, crate_name, pkg_descr, build_script_outputs, output_file, script_out_dir) = all; let output = match prev_output { Some(output) => output, None => BuildOutput::parse_file( &output_file, - pkg_name, + crate_name, &pkg_descr, &prev_script_out_dir, &script_out_dir, @@ -479,7 +478,7 @@ fn insert_warnings_in_build_outputs( impl BuildOutput { pub fn parse_file( path: &Path, - pkg_name: InternedString, + crate_name: String, pkg_descr: &str, script_out_dir_when_generated: &Path, script_out_dir: &Path, @@ -489,7 +488,7 @@ impl BuildOutput { let contents = paths::read_bytes(path)?; BuildOutput::parse( &contents, - pkg_name, + crate_name, pkg_descr, script_out_dir_when_generated, script_out_dir, @@ -499,10 +498,12 @@ impl BuildOutput { } // Parses the output of a script. - // The `pkg_name` is used for error messages. + // The `pkg_descr` is used for error messages. + // The `crate_name` is used for determining if RUSTC_BOOTSTRAP should be allowed. pub fn parse( input: &[u8], - pkg_name: InternedString, + // Takes String instead of InternedString so passing `unit.pkg.name()` will give a compile error. + crate_name: String, pkg_descr: &str, script_out_dir_when_generated: &Path, script_out_dir: &Path, @@ -591,13 +592,12 @@ impl BuildOutput { // behavior, so still only give a warning. // NOTE: cargo only allows nightly features on RUSTC_BOOTSTRAP=1, but we // want setting any value of RUSTC_BOOTSTRAP to downgrade this to a warning - // (so that `RUSTC_BOOTSTRAP=pkg_name` will work) + // (so that `RUSTC_BOOTSTRAP=crate_name` will work) let rustc_bootstrap_allows = |name: &str| { - std::env::var("RUSTC_BOOTSTRAP").map_or(false, |var| { - var.split(',').any(|s| s == name) - }) + std::env::var("RUSTC_BOOTSTRAP") + .map_or(false, |var| var.split(',').any(|s| s == name)) }; - if nightly_features_allowed || rustc_bootstrap_allows(&*pkg_name) { + if nightly_features_allowed || rustc_bootstrap_allows(&*crate_name) { warnings.push(format!("Cannot set `RUSTC_BOOTSTRAP={}` from {}.\n\ note: Crates cannot set `RUSTC_BOOTSTRAP` themselves, as doing so would subvert the stability guarantees of Rust for your project.", val, whence @@ -610,7 +610,7 @@ impl BuildOutput { help: If you're sure you want to do this in your project, set the environment variable `RUSTC_BOOTSTRAP={}` before running cargo instead.", val, whence, - pkg_name, + crate_name, ); } } else { @@ -867,7 +867,7 @@ fn prev_build_output(cx: &mut Context<'_, '_>, unit: &Unit) -> (Option Date: Sat, 17 Apr 2021 11:31:48 -0400 Subject: [PATCH 3/5] Use the library name to decide whether the override should be allowed from the top-level. If there's no library, give a hard error unless features are unconditionally allowed with RUSTC_BOOTSTRAP=1. --- src/cargo/core/compiler/custom_build.rs | 46 +++++++++++++++++-------- tests/testsuite/build_script_env.rs | 43 +++++++++++++++++------ 2 files changed, 64 insertions(+), 25 deletions(-) diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs index 4004c000f5d..ac824329a3e 100644 --- a/src/cargo/core/compiler/custom_build.rs +++ b/src/cargo/core/compiler/custom_build.rs @@ -269,7 +269,12 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { } }) .collect::>(); - let crate_name = unit.target.crate_name(); + let library_name = unit + .pkg + .targets() + .iter() + .find(|t| t.is_lib()) + .map(|t| t.crate_name()); let pkg_descr = unit.pkg.to_string(); let build_script_outputs = Arc::clone(&cx.build_script_outputs); let id = unit.pkg.package_id(); @@ -279,7 +284,7 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { let host_target_root = cx.files().host_dest().to_path_buf(); let all = ( id, - crate_name.clone(), + library_name.clone(), pkg_descr.clone(), Arc::clone(&build_script_outputs), output_file.clone(), @@ -399,7 +404,7 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { paths::write(&root_output_file, paths::path2bytes(&script_out_dir)?)?; let parsed_output = BuildOutput::parse( &output.stdout, - crate_name, + library_name, &pkg_descr, &script_out_dir, &script_out_dir, @@ -421,12 +426,12 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { // itself to run when we actually end up just discarding what we calculated // above. let fresh = Work::new(move |state| { - let (id, crate_name, pkg_descr, build_script_outputs, output_file, script_out_dir) = all; + let (id, library_name, pkg_descr, build_script_outputs, output_file, script_out_dir) = all; let output = match prev_output { Some(output) => output, None => BuildOutput::parse_file( &output_file, - crate_name, + library_name, &pkg_descr, &prev_script_out_dir, &script_out_dir, @@ -478,7 +483,7 @@ fn insert_warnings_in_build_outputs( impl BuildOutput { pub fn parse_file( path: &Path, - crate_name: String, + library_name: Option, pkg_descr: &str, script_out_dir_when_generated: &Path, script_out_dir: &Path, @@ -488,7 +493,7 @@ impl BuildOutput { let contents = paths::read_bytes(path)?; BuildOutput::parse( &contents, - crate_name, + library_name, pkg_descr, script_out_dir_when_generated, script_out_dir, @@ -499,11 +504,11 @@ impl BuildOutput { // Parses the output of a script. // The `pkg_descr` is used for error messages. - // The `crate_name` is used for determining if RUSTC_BOOTSTRAP should be allowed. + // The `library_name` is used for determining if RUSTC_BOOTSTRAP should be allowed. pub fn parse( input: &[u8], // Takes String instead of InternedString so passing `unit.pkg.name()` will give a compile error. - crate_name: String, + library_name: Option, pkg_descr: &str, script_out_dir_when_generated: &Path, script_out_dir: &Path, @@ -592,12 +597,21 @@ impl BuildOutput { // behavior, so still only give a warning. // NOTE: cargo only allows nightly features on RUSTC_BOOTSTRAP=1, but we // want setting any value of RUSTC_BOOTSTRAP to downgrade this to a warning - // (so that `RUSTC_BOOTSTRAP=crate_name` will work) - let rustc_bootstrap_allows = |name: &str| { + // (so that `RUSTC_BOOTSTRAP=library_name` will work) + let rustc_bootstrap_allows = |name: Option<&str>| { + let name = match name { + // as of 2021, no binaries on crates.io use RUSTC_BOOTSTRAP, so + // fine-grained opt-outs aren't needed. end-users can always use + // RUSTC_BOOTSTRAP=1 from the top-level if it's really a problem. + None => return false, + Some(n) => n, + }; std::env::var("RUSTC_BOOTSTRAP") .map_or(false, |var| var.split(',').any(|s| s == name)) }; - if nightly_features_allowed || rustc_bootstrap_allows(&*crate_name) { + if nightly_features_allowed + || rustc_bootstrap_allows(library_name.as_deref()) + { warnings.push(format!("Cannot set `RUSTC_BOOTSTRAP={}` from {}.\n\ note: Crates cannot set `RUSTC_BOOTSTRAP` themselves, as doing so would subvert the stability guarantees of Rust for your project.", val, whence @@ -610,7 +624,7 @@ impl BuildOutput { help: If you're sure you want to do this in your project, set the environment variable `RUSTC_BOOTSTRAP={}` before running cargo instead.", val, whence, - crate_name, + library_name.as_deref().unwrap_or("1"), ); } } else { @@ -867,7 +881,11 @@ fn prev_build_output(cx: &mut Context<'_, '_>, unit: &Unit) -> (Option Date: Sat, 17 Apr 2021 11:33:51 -0400 Subject: [PATCH 4/5] minor cleanup --- src/cargo/core/compiler/custom_build.rs | 13 ++----------- src/cargo/core/package.rs | 4 ++++ 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs index ac824329a3e..1587f100eae 100644 --- a/src/cargo/core/compiler/custom_build.rs +++ b/src/cargo/core/compiler/custom_build.rs @@ -269,12 +269,7 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { } }) .collect::>(); - let library_name = unit - .pkg - .targets() - .iter() - .find(|t| t.is_lib()) - .map(|t| t.crate_name()); + let library_name = unit.pkg.library().map(|t| t.crate_name()); let pkg_descr = unit.pkg.to_string(); let build_script_outputs = Arc::clone(&cx.build_script_outputs); let id = unit.pkg.package_id(); @@ -881,11 +876,7 @@ fn prev_build_output(cx: &mut Context<'_, '_>, unit: &Unit) -> (Option &[Target] { self.manifest().targets() } + /// Gets the library crate for this package, if it exists. + pub fn library(&self) -> Option<&Target> { + self.targets().iter().find(|t| t.is_lib()) + } /// Gets the current package version. pub fn version(&self) -> &Version { self.package_id().version() From 5a7149683f8c61292d8d524304e9c248f683d993 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 17 Apr 2021 11:59:41 -0400 Subject: [PATCH 5/5] Fix tests --- tests/testsuite/build_script_env.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/testsuite/build_script_env.rs b/tests/testsuite/build_script_env.rs index d4cb7ca2d7f..17c729bc9c1 100644 --- a/tests/testsuite/build_script_env.rs +++ b/tests/testsuite/build_script_env.rs @@ -128,9 +128,12 @@ fn rustc_bootstrap() { ) .with_status(101) .run(); - // RUSTC_BOOTSTRAP unset on nightly should warn + // nightly should warn whether or not RUSTC_BOOTSTRAP is set p.cargo("build") .masquerade_as_nightly_cargo() + // NOTE: uses RUSTC_BOOTSTRAP so it will be propagated to rustc + // (this matters when tests are being run with a beta or stable cargo) + .env("RUSTC_BOOTSTRAP", "1") .with_stderr_contains("warning: Cannot set `RUSTC_BOOTSTRAP=1` [..]") .run(); // RUSTC_BOOTSTRAP set to the name of the library should warn @@ -151,21 +154,22 @@ fn rustc_bootstrap() { // Tests for binaries instead of libraries let p = project() .file("Cargo.toml", &basic_manifest("foo", "0.0.1")) - .file("src/main.rs", "#![feature(rustc_attrs)] fn main()") + .file("src/main.rs", "#![feature(rustc_attrs)] fn main() {}") .file("build.rs", build_rs) .build(); - // RUSTC_BOOTSTRAP unconditionally set when there's no library should warn + // nightly should warn when there's no library whether or not RUSTC_BOOTSTRAP is set p.cargo("build") .masquerade_as_nightly_cargo() + // NOTE: uses RUSTC_BOOTSTRAP so it will be propagated to rustc + // (this matters when tests are being run with a beta or stable cargo) + .env("RUSTC_BOOTSTRAP", "1") .with_stderr_contains("warning: Cannot set `RUSTC_BOOTSTRAP=1` [..]") .run(); // RUSTC_BOOTSTRAP conditionally set when there's no library should error (regardless of the value) p.cargo("build") .env("RUSTC_BOOTSTRAP", "foo") .with_stderr_contains("error: Cannot set `RUSTC_BOOTSTRAP=1` [..]") - .with_stderr_does_not_contain( - "help: [..] set the environment variable `RUSTC_BOOTSTRAP=1` [..]", - ) + .with_stderr_contains("help: [..] set the environment variable `RUSTC_BOOTSTRAP=1` [..]") .with_status(101) .run(); }