Skip to content

Commit

Permalink
Auto merge of #9365 - jyn514:rustc-bootstrap-crate-name, r=ehuss
Browse files Browse the repository at this point in the history
Don't give a hard error when the end-user specifies RUSTC_BOOTSTRAP=crate_name

Fixes #9362.
The whole point of rust-lang/rust#77802 was to allow specifying this granularly, giving a hard error defeats the point.

I didn't know how to check what targets were reverse-dependencies of build.rs, so I just unconditionally use the library name (and give a hard error for anything else, even if it's the name of one of the binaries). End-users can still opt-in with RUSTC_BOOTSTRAP=1, and no public binaries use RUSTC_BOOTSTRAP=1, so I don't think this a big deal in practice.

<details><summary>Script to verify all crates using RUSTC_BOOTSTRAP=1 have a library</summary>

```sh
curl https://pastebin.com/raw/fGQ97xP6 | cut -d / -f1 | grep -v shnatsel | grep -v cargo- | sed 's#-\([0-9]\)#/\1#' | xargs -i curl -s -I -L "https://docs.rs/{}/" -w "%{http_code}\n" -o/dev/null
```

It should output 20 200s in a row.

</details>

r? `@ehuss` cc `@mark-simulacrum`

I don't know what cargo's policy is for backports, but this should be backported to 1.52.
  • Loading branch information
bors committed Apr 20, 2021
2 parents a18936b + 5a71496 commit fb0130c
Show file tree
Hide file tree
Showing 3 changed files with 87 additions and 23 deletions.
43 changes: 30 additions & 13 deletions src/cargo/core/compiler/custom_build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 _;
Expand Down Expand Up @@ -270,7 +269,7 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Job> {
}
})
.collect::<Vec<_>>();
let pkg_name = unit.pkg.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();
Expand All @@ -280,7 +279,7 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Job> {
let host_target_root = cx.files().host_dest().to_path_buf();
let all = (
id,
pkg_name,
library_name.clone(),
pkg_descr.clone(),
Arc::clone(&build_script_outputs),
output_file.clone(),
Expand Down Expand Up @@ -400,7 +399,7 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Job> {
paths::write(&root_output_file, paths::path2bytes(&script_out_dir)?)?;
let parsed_output = BuildOutput::parse(
&output.stdout,
pkg_name,
library_name,
&pkg_descr,
&script_out_dir,
&script_out_dir,
Expand All @@ -422,12 +421,12 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Job> {
// 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, 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,
pkg_name,
library_name,
&pkg_descr,
&prev_script_out_dir,
&script_out_dir,
Expand Down Expand Up @@ -479,7 +478,7 @@ fn insert_warnings_in_build_outputs(
impl BuildOutput {
pub fn parse_file(
path: &Path,
pkg_name: InternedString,
library_name: Option<String>,
pkg_descr: &str,
script_out_dir_when_generated: &Path,
script_out_dir: &Path,
Expand All @@ -489,7 +488,7 @@ impl BuildOutput {
let contents = paths::read_bytes(path)?;
BuildOutput::parse(
&contents,
pkg_name,
library_name,
pkg_descr,
script_out_dir_when_generated,
script_out_dir,
Expand All @@ -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 `library_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.
library_name: Option<String>,
pkg_descr: &str,
script_out_dir_when_generated: &Path,
script_out_dir: &Path,
Expand Down Expand Up @@ -589,7 +590,23 @@ 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=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(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
Expand All @@ -602,7 +619,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,
library_name.as_deref().unwrap_or("1"),
);
}
} else {
Expand Down Expand Up @@ -859,7 +876,7 @@ fn prev_build_output(cx: &mut Context<'_, '_>, unit: &Unit) -> (Option<BuildOutp
(
BuildOutput::parse_file(
&output_file,
unit.pkg.name(),
unit.pkg.library().map(|t| t.crate_name()),
&unit.pkg.to_string(),
&prev_script_out_dir,
&script_out_dir,
Expand Down
4 changes: 4 additions & 0 deletions src/cargo/core/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,10 @@ impl Package {
pub fn targets(&self) -> &[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()
Expand Down
63 changes: 53 additions & 10 deletions tests/testsuite/build_script_env.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Tests for build.rs rerun-if-env-changed and rustc-env

use cargo_test_support::basic_manifest;
use cargo_test_support::project;
use cargo_test_support::sleep_ms;

Expand Down Expand Up @@ -109,24 +110,66 @@ fn rerun_if_env_or_file_changes() {

#[cargo_test]
fn rustc_bootstrap() {
let build_rs = r#"
fn main() {
println!("cargo:rustc-env=RUSTC_BOOTSTRAP=1");
}
"#;
let p = project()
.file("src/main.rs", "fn main() {}")
.file(
"build.rs",
r#"
fn main() {
println!("cargo:rustc-env=RUSTC_BOOTSTRAP=1");
}
"#,
)
.file("Cargo.toml", &basic_manifest("has-dashes", "0.0.1"))
.file("src/lib.rs", "#![feature(rustc_attrs)]")
.file("build.rs", build_rs)
.build();
// RUSTC_BOOTSTRAP unset on stable should error
p.cargo("build")
.with_stderr_contains("error: Cannot set `RUSTC_BOOTSTRAP=1` [..]")
.with_stderr_contains("help: [..] set the environment variable `RUSTC_BOOTSTRAP=foo` [..]")
.with_stderr_contains(
"help: [..] set the environment variable `RUSTC_BOOTSTRAP=has_dashes` [..]",
)
.with_status(101)
.run();
// 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
p.cargo("build")
.env("RUSTC_BOOTSTRAP", "has_dashes")
.with_stderr_contains("warning: Cannot set `RUSTC_BOOTSTRAP=1` [..]")
.run();
// RUSTC_BOOTSTRAP set to some random value should error
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=has_dashes` [..]",
)
.with_status(101)
.run();

// 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("build.rs", build_rs)
.build();
// 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_contains("help: [..] set the environment variable `RUSTC_BOOTSTRAP=1` [..]")
.with_status(101)
.run();
}

0 comments on commit fb0130c

Please sign in to comment.