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.

Sign up for GitHub

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

Jump to bottom

Don't give a hard error when the end-user specifies RUSTC_BOOTSTRAP=crate_name #9365

Merged
merged 5 commits into from
Apr 21, 2021
Merged

Don't give a hard error when the end-user specifies RUSTC_BOOTSTRAP=crate_name #9365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
04fa2f2
Don't give a hard error on RUSTC_BOOTSTRAP=crate_name
jyn514 Apr 16, 2021
6d6f466
Use the crate name instead of the package name
jyn514 Apr 16, 2021
236fe87
Use the library name to decide whether the override should be allowed…
jyn514 Apr 17, 2021
1b36ddc
minor cleanup
jyn514 Apr 17, 2021
5a71496
Fix tests
jyn514 Apr 17, 2021
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
43 changes: 30 additions & 13 deletions src/cargo/core/compiler/custom_build.rs
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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();
}