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

Support per-pkg-target for -Zbuild-std, take two #11969

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
65 changes: 44 additions & 21 deletions src/cargo/core/compiler/standard_lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use crate::core::{Dependency, PackageId, PackageSet, Resolve, SourceId, Workspac
use crate::ops::{self, Packages};
use crate::util::errors::CargoResult;
use crate::Config;
use std::collections::hash_map::Entry;
use std::collections::{HashMap, HashSet};
use std::path::PathBuf;

Expand Down Expand Up @@ -168,7 +169,8 @@ pub fn generate_std_roots(
crates: &[String],
std_resolve: &Resolve,
std_features: &ResolvedFeatures,
kinds: &[CompileKind],
requested_kinds: &[CompileKind],
explicit_host_kind: CompileKind,
package_set: &PackageSet<'_>,
interner: &UnitInterner,
profiles: &Profiles,
Expand All @@ -182,41 +184,62 @@ pub fn generate_std_roots(
let std_pkgs = package_set.get_many(std_ids)?;
// Generate a map of Units for each kind requested.
let mut ret = HashMap::new();
for pkg in std_pkgs {
let lib = pkg
.targets()
.iter()
.find(|t| t.is_lib())
.expect("std has a lib");
// I don't think we need to bother with Check here, the difference
// in time is minimal, and the difference in caching is
// significant.
let mode = CompileMode::Build;
let features = std_features.activated_features(pkg.package_id(), FeaturesFor::NormalOrDev);
for kind in kinds {
let list = ret.entry(*kind).or_insert_with(Vec::new);
let unit_for = UnitFor::new_normal(*kind);

// Get information for the list of packages for standard library
let std_pkg_infos: Vec<_> = std_pkgs
.iter()
.map(|pkg| {
let lib = pkg
.targets()
.iter()
.find(|t| t.is_lib())
.expect("std has a lib");
let features =
std_features.activated_features(pkg.package_id(), FeaturesFor::NormalOrDev);
let unit_for = UnitFor::new_normal(explicit_host_kind);
(pkg, lib, unit_for, features)
})
.collect();

// Iterate over all packages over the package set to find all targets requested.
for kind in package_set
.packages()
.flat_map(|pkg| pkg.explicit_kinds(requested_kinds, explicit_host_kind))
{
let e = match ret.entry(kind) {
Entry::Vacant(e) => e,
Entry::Occupied(_) => continue,
};

let units = std_pkg_infos.iter().map(|(pkg, lib, unit_for, features)| {
// I don't think we need to bother with Check here, the difference
// in time is minimal, and the difference in caching is
// significant.
let mode = CompileMode::Build;
let profile = profiles.get_profile(
pkg.package_id(),
/*is_member*/ false,
/*is_local*/ false,
unit_for,
*kind,
*unit_for,
kind,
);
list.push(interner.intern(
interner.intern(
pkg,
lib,
profile,
*kind,
kind,
mode,
features.clone(),
/*is_std*/ true,
/*dep_hash*/ 0,
IsArtifact::No,
None,
));
}
)
});

e.insert(units.collect());
}

Ok(ret)
}

Expand Down
22 changes: 22 additions & 0 deletions src/cargo/core/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,28 @@ impl Package {
self.targets().iter().any(|t| t.is_custom_build())
}

/// Returns explicit kinds either forced by `forced-target` in `Cargo.toml`,
/// fallback to `default-target`, or specified in cli parameters.
pub fn explicit_kinds(
&self,
requested_kinds: &[CompileKind],
explicit_host_kind: CompileKind,
) -> Vec<CompileKind> {
if let Some(k) = self.manifest().forced_kind() {
vec![k]
} else {
requested_kinds
.iter()
.map(|kind| match kind {
CompileKind::Host => {
self.manifest().default_kind().unwrap_or(explicit_host_kind)
}
CompileKind::Target(t) => CompileKind::Target(*t),
})
.collect()
}
}

pub fn map_source(self, to_replace: SourceId, replace_with: SourceId) -> Package {
Package {
inner: Rc::new(PackageInner {
Expand Down
11 changes: 2 additions & 9 deletions src/cargo/ops/cargo_compile/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -341,14 +341,6 @@ pub fn create_bcx<'a, 'cfg>(
// assuming `--target $HOST` was specified. See
// `rebuild_unit_graph_shared` for more on why this is done.
let explicit_host_kind = CompileKind::Target(CompileTarget::new(&target_data.rustc.host)?);
let explicit_host_kinds: Vec<_> = build_config
.requested_kinds
.iter()
.map(|kind| match kind {
CompileKind::Host => explicit_host_kind,
CompileKind::Target(t) => CompileKind::Target(*t),
})
.collect();

// Passing `build_config.requested_kinds` instead of
// `explicit_host_kinds` here so that `generate_root_units` can do
Comment on lines 345 to 346
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a comment that looks like it needs to be updated here due to the removal of explicit_host_kinds.

Expand Down Expand Up @@ -393,7 +385,8 @@ pub fn create_bcx<'a, 'cfg>(
&crates,
std_resolve,
std_features,
&explicit_host_kinds,
&build_config.requested_kinds,
explicit_host_kind,
&pkg_set,
interner,
&profiles,
Expand Down
15 changes: 1 addition & 14 deletions src/cargo/ops/cargo_compile/unit_generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,20 +111,7 @@ impl<'a> UnitGenerator<'a, '_> {
// why this is done. However, if the package has its own
// `package.target` key, then this gets used instead of
// `$HOST`
let explicit_kinds = if let Some(k) = pkg.manifest().forced_kind() {
vec![k]
} else {
self.requested_kinds
.iter()
.map(|kind| match kind {
CompileKind::Host => pkg
.manifest()
.default_kind()
.unwrap_or(self.explicit_host_kind),
CompileKind::Target(t) => CompileKind::Target(*t),
})
.collect()
};
let explicit_kinds = pkg.explicit_kinds(self.requested_kinds, self.explicit_host_kind);

explicit_kinds
.into_iter()
Expand Down
47 changes: 47 additions & 0 deletions tests/build-std/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -227,3 +227,50 @@ fn custom_test_framework() {
.build_std_arg("core")
.run();
}

/// like cross-custom but uses per-package-target instead
#[cargo_test(build_std_real)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you say why this needs to be a real build-std test? I would prefer to avoid those if possible. The mock tests should be able to use .arg("target").arg(alternate()) and use the alternate function to specify a target different from the host (and not using a JSON target).

fn per_package_target() {
let p = project()
.file(
"Cargo.toml",
r#"
cargo-features = ["per-package-target"]
[package]
name = "foo"
version = "0.1.0"
edition = "2018"
default-target = "custom-target.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is testing this target. Because build-std requires --target to be specified, the default target is never used. I might suggest using forced-target.

# [target.custom-target.dependencies] <-- not sure why this doesn't work
[dependencies]
dep = { path = "dep" }
"#,
)
.file(
"src/lib.rs",
"#![no_std] pub fn f() -> u32 { dep::answer() }",
)
.file("dep/Cargo.toml", &basic_manifest("dep", "0.1.0"))
.file("dep/src/lib.rs", "#![no_std] pub fn answer() -> u32 { 42 }")
.file(
"custom-target.json",
r#"
{
"llvm-target": "x86_64-unknown-none-gnu",
"data-layout": "e-m:e-i64:64-f80:128-n8:16:32:64-S128",
"arch": "x86_64",
"target-endian": "little",
"target-pointer-width": "64",
"target-c-int-width": "32",
"os": "none",
"linker-flavor": "ld.lld"
}
"#,
)
.build();

p.cargo("build -v")
.build_std_arg("core")
Copy link
Contributor

Choose a reason for hiding this comment

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

Preferably this would contain a with_stderr to validate that the --target flag is being passed correctly for each invocation.

.target_host()
.run();
}
7 changes: 6 additions & 1 deletion tests/testsuite/mock-std/library/proc_macro/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
#![feature(staged_api)]
#![stable(since = "1.0.0", feature = "dummy")]


extern crate proc_macro;

// Don't re-export everything in the root so that the mock std can be distinguished from the real one.
#[stable(since = "1.0.0", feature = "dummy")]
pub use proc_macro::*;
pub mod exported {
#[stable(since = "1.0.0", feature = "dummy")]
pub use proc_macro::*;
}
Comment on lines +7 to +12
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify why this is needed?

In general, I'm not seeing the connection with proc-macro changes and support for per-pkg-target.


#[stable(since = "1.0.0", feature = "dummy")]
pub fn custom_api() {
Expand Down
99 changes: 98 additions & 1 deletion tests/testsuite/standard_lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -366,16 +366,113 @@ fn target_proc_macro() {
"src/lib.rs",
r#"
extern crate proc_macro;
pub fn f() {
fn f() {
let _ts = proc_macro::TokenStream::new();
}
"#,
)
.file(
"Cargo.toml",
r#"
[package]
name = "pm"
version = "0.1.0"
[lib]
proc-macro = true
"#,
)
Comment on lines +374 to +383
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you say why this was added?

.build();

p.cargo("build -v").build_std(&setup).target_host().run();
}

// We already have `basic` which uses `proc_macro::custom_api()`. This case attempts to use
// `TokenStream` which would error because we are using the sysroot version.
#[cargo_test(build_std_mock)]
fn non_proc_macro_crate_uses_non_sysroot_proc_macro() {
let setup = setup();

let p = project()
.file(
"src/lib.rs",
r#"
extern crate proc_macro;
fn f() {
let _ts = proc_macro::TokenStream::new();
}
"#,
)
.file(
"Cargo.toml",
r#"
[package]
name = "pm"
version = "0.1.0"
"#,
)
.build();

p.cargo("build -v")
.build_std(&setup)
.target_host()
.run_expect_error();
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use with_status(), and also have with_stderr() to verify which error is happening.

Also, similar to the other questions, I'm not clear why this should fail. What's wrong with having a library access the proc_macro crate?

}

#[cargo_test(build_std_mock)]
fn intergrated_proc_macro() {
let setup = setup();

let p = project()
.file(
"src/main.rs",
r#"
fn main() {
println!("The answer is {}", pm::m!());
}
"#,
)
.file(
"pm/src/lib.rs",
r#"
extern crate proc_macro;
use proc_macro::TokenStream;
#[proc_macro]
pub fn m(_item: TokenStream) -> TokenStream {
"42".parse().unwrap()
}
"#,
)
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"
[workspace]
members = ["pm"]
[dependencies]
pm = { path = "./pm" }
"#,
)
.file(
"pm/Cargo.toml",
r#"
[package]
name = "pm"
version = "0.1.0"
[lib]
proc-macro = true
"#,
)
.build();

p.cargo("run -v")
.build_std(&setup)
.with_stdout_contains("The answer is 42")
.target_host()
.run();
}

#[cargo_test(build_std_mock)]
fn bench() {
let setup = setup();
Expand Down