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

Change build-std to use --sysroot #7421

Merged
merged 8 commits into from
Sep 25, 2019
Merged
Show file tree
Hide file tree
Changes from 7 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
12 changes: 12 additions & 0 deletions crates/cargo-test-support/src/paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use std::env;
use std::fs;
use std::io::{self, ErrorKind};
use std::path::{Path, PathBuf};
use std::process::Command;
use std::sync::atomic::{AtomicUsize, Ordering};
use std::sync::Mutex;

Expand Down Expand Up @@ -252,3 +253,14 @@ pub fn get_lib_extension(kind: &str) -> &str {
_ => unreachable!(),
}
}

/// Returns the sysroot as queried from rustc.
pub fn sysroot() -> String {
let output = Command::new("rustc")
.arg("--print=sysroot")
.output()
.expect("rustc to run");
assert!(output.status.success());
let sysroot = String::from_utf8(output.stdout).unwrap();
sysroot.trim().to_string()
}
8 changes: 7 additions & 1 deletion src/cargo/core/compiler/compilation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,13 @@ impl<'cfg> Compilation<'cfg> {
super::filter_dynamic_search_path(self.native_dirs.iter(), &self.root_output);
search_path.push(self.deps_output.clone());
search_path.push(self.root_output.clone());
search_path.push(self.target_dylib_path.clone());
// For build-std, we don't want to accidentally pull in any shared
// libs from the sysroot that ships with rustc. This may not be
// required (at least I cannot craft a situation where it
// matters), but is here to be safe.
if self.config.cli_unstable().build_std.is_none() {
search_path.push(self.target_dylib_path.clone());
}
search_path
};

Expand Down
7 changes: 6 additions & 1 deletion src/cargo/core/compiler/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use super::custom_build::{self, BuildDeps, BuildScriptOutputs, BuildScripts};
use super::fingerprint::Fingerprint;
use super::job_queue::JobQueue;
use super::layout::Layout;
use super::standard_lib;
use super::unit_dependencies::{UnitDep, UnitGraph};
use super::{BuildContext, Compilation, CompileMode, Executor, FileFlavor, Kind};

Expand Down Expand Up @@ -304,7 +305,11 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
};
let host_layout = Layout::new(self.bcx.ws, None, dest)?;
let target_layout = match self.bcx.build_config.requested_target.as_ref() {
Some(target) => Some(Layout::new(self.bcx.ws, Some(target), dest)?),
Some(target) => {
let layout = Layout::new(self.bcx.ws, Some(target), dest)?;
standard_lib::prepare_sysroot(&layout)?;
Some(layout)
}
None => None,
};
self.primary_packages
Expand Down
7 changes: 6 additions & 1 deletion src/cargo/core/compiler/job_queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use super::job::{
Freshness::{self, Dirty, Fresh},
Job,
};
use super::standard_lib;
use super::timings::Timings;
use super::{BuildContext, BuildPlan, CompileMode, Context, Unit};
use crate::core::{PackageId, TargetKind};
Expand Down Expand Up @@ -607,7 +608,7 @@ impl<'a, 'cfg> JobQueue<'a, 'cfg> {
id: u32,
unit: &Unit<'a>,
artifact: Artifact,
cx: &mut Context<'_, '_>,
cx: &mut Context<'a, '_>,
) -> CargoResult<()> {
if unit.mode.is_run_custom_build() && cx.bcx.show_warnings(unit.pkg.package_id()) {
self.emit_warnings(None, unit, cx)?;
Expand All @@ -617,6 +618,10 @@ impl<'a, 'cfg> JobQueue<'a, 'cfg> {
Artifact::All => self.timings.unit_finished(id, unlocked),
Artifact::Metadata => self.timings.unit_rmeta_finished(id, unlocked),
}
if unit.is_std && unit.kind == super::Kind::Target && !cx.bcx.build_config.build_plan {
let rmeta = artifact == Artifact::Metadata;
standard_lib::add_sysroot_artifact(cx, unit, rmeta)?;
}
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to sink this work into the unit itself? Ideally we wouldn't be doing much extra stuff here in job_queue.rs and it keeps all the major work on each thread cargo spawns for work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how that would work, particularly with pipelining. The rmeta json artifact notification goes through a generic route that most conveniently ends up here in JobQueue. I could disable pipelining and move it to the Work closure instead?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see what you mean. Thinking more on this though, I was actually assuming that we'd just change --out-dir for is_std crates. It looks like otherwise they're gonna show up both in the libdir as well as the main deps folder, right? If we switched --out-dir I think it'd solve this issue naturally?

Copy link
Member

Choose a reason for hiding this comment

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

Er wait no we already talked about it and ruled it out because we want to clean out the sysroot on each build.

In any case this poses an interesting scenario where the dependencies of std can be loaded from either the sysroot or -Ldependency=.../deps, they're actually all the same. I'm asking this from the point of view of concurrency which I think this issue is trying to solve (making sure we do this before the next compilation starts), but maybe that's not actually necessary?

Ok I want to try to work this through here in a coment...

  • So for std->std dependencies, as well as non-std->non-std dependencies, those all use --extern I believe. If that's the case then I think that nothing is necessary here in terms of sequencing things right because everything is loaded from deps
  • The only case I think this cares about is non-std -> std dependencies, since they're loaded through the sysroot. Specifically you're worried here that we will kick off compilations that depend on core, but nothing may actually be in the sysroot yet, so the compiler won't be able to find anything.

Does that sound right? If so...

So presumably this would otherwise go somewhere around here (transitively), and you're right that's a "pretty generic path" which isn't a great spot to put it.

Ok if you agree with what I've said above, let's leave this here, but add a comment perhaps saying that we may want to figure out how to offload it to worker threads eventually?

Copy link
Contributor Author

@ehuss ehuss Sep 24, 2019

Choose a reason for hiding this comment

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

That would be cleaner. For some reason I was thinking that was going to be harder (since the output directory is somewhat global), but it probably won't be too hard. I'll give it a shot.

EDIT: Oh, and I think I was also concerned about not atomically adding files while concurrent rustc's are running, but you mentioned that it shouldn't be too much of a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we want to clean out the sysroot on each build

Oh, yea, that's the actual reason. Sorry, I'm mis-remembering things.

Ok if you agree with what I've said above, let's leave this here, but add a comment perhaps saying that we may want to figure out how to offload it to worker threads eventually?

That makes sense. I'll add a comment.

I was previously trying to think of a way that OutputFile::hard_link (or something like it) could be used, but it still runs into the problem with rmeta files needing to be available, and linking isn't done until after compilation finishes.

Ok(())
}

Expand Down
59 changes: 49 additions & 10 deletions src/cargo/core/compiler/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
//! .rustc-info.json
//!
//! # All final artifacts are linked into this directory from `deps`.
//! # Note that named profiles will soon be included as separate directories
//! # here. They have a restricted format, similar to Rust identifiers, so
//! # Cargo-specific directories added in the future should use some prefix
//! # like `.` to avoid name collisions.
//! debug/ # or release/
//!
//! # File used to lock the directory to prevent multiple cargo processes
Expand Down Expand Up @@ -46,6 +50,11 @@
//! # incremental is enabled.
//! incremental/
//!
//! # The sysroot for -Zbuild-std builds. This only appears in
//! # target-triple directories (not host), and only if -Zbuild-std is
//! # enabled.
//! .sysroot/
//!
//! # This is the location at which the output of all custom build
//! # commands are rooted.
//! build/
Expand Down Expand Up @@ -116,6 +125,10 @@ pub struct Layout {
examples: PathBuf,
/// The directory for rustdoc output: `$root/doc`
doc: PathBuf,
/// The local sysroot for the build-std feature.
sysroot: Option<PathBuf>,
/// The "lib" directory within `sysroot`.
sysroot_libdir: Option<PathBuf>,
/// The lockfile for a build (`.cargo-lock`). Will be unlocked when this
/// struct is `drop`ped.
_lock: FileLock,
Expand All @@ -139,18 +152,21 @@ impl Layout {
// Flexible target specifications often point at json files, so interpret
// the target triple as a Path and then just use the file stem as the
// component for the directory name in that case.
if let Some(triple) = triple {
let triple = Path::new(triple);
if triple.extension().and_then(|s| s.to_str()) == Some("json") {
root.push(
triple
.file_stem()
let triple_path = if let Some(s) = triple {
let p = Path::new(s);
let tp = if p.extension().and_then(|s| s.to_str()) == Some("json") {
Path::new(
p.file_stem()
.ok_or_else(|| failure::format_err!("invalid target"))?,
);
)
} else {
root.push(triple);
}
}
p
};
root.push(tp);
Some(tp)
} else {
None
};
let dest = root.join(dest);
// If the root directory doesn't already exist go ahead and create it
// here. Use this opportunity to exclude it from backups as well if the
Expand All @@ -167,6 +183,17 @@ impl Layout {
let root = root.into_path_unlocked();
let dest = dest.into_path_unlocked();

// Compute the sysroot path for the build-std feature.
let build_std = ws.config().cli_unstable().build_std.as_ref();
let (sysroot, sysroot_libdir) = if let Some(tp) = build_std.and(triple_path) {
// This uses a leading dot to avoid collision with named profiles.
let sysroot = dest.join(".sysroot");
Copy link
Member

Choose a reason for hiding this comment

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

I'd have a mild preference for not hiding this directory by default, but I'm curious why you added the leading period?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is that named profiles will be taking over this namespace (each named profile has a directory with its name). I'd like to avoid potential conflicts. An alternative would be to reserve "sysroot" and not allow it as a profile name (which won't be much of an option once named profiles stabilizes). Or we could come up with some other structure or naming convention. Any preferences?

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok sounds good to me! Can you add that to a comment indicating why there's a leading dot?

let sysroot_libdir = sysroot.join("lib").join("rustlib").join(tp).join("lib");
Copy link
Member

Choose a reason for hiding this comment

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

So technically this isn't going to work on all compilers digging in a bit. The sysroot finding function in rustc takes into account CFG_LIBDIR_RELATIVE which is set by taking the difference between --libdir and --prefix when building rustc itself.

Now that's pretty esoteric though and only really relevant for Linux distributions, but it may be worth keeping in mind that this may run afoul of that at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh… I'll try to investigate. There doesn't seem to be a way to discover the actual path.

(Some(sysroot), Some(sysroot_libdir))
} else {
(None, None)
};

Ok(Layout {
deps: dest.join("deps"),
build: dest.join("build"),
Expand All @@ -176,6 +203,8 @@ impl Layout {
doc: root.join("doc"),
root,
dest,
sysroot,
sysroot_libdir,
_lock: lock,
})
}
Expand Down Expand Up @@ -223,6 +252,16 @@ impl Layout {
pub fn build(&self) -> &Path {
&self.build
}
/// The local sysroot for the build-std feature.
///
/// Returns None if build-std is not enabled or this is the Host layout.
pub fn sysroot(&self) -> Option<&Path> {
self.sysroot.as_ref().map(|p| p.as_ref())
}
/// The "lib" directory within `sysroot`.
pub fn sysroot_libdir(&self) -> Option<&Path> {
self.sysroot_libdir.as_ref().map(|p| p.as_ref())
}
}

#[cfg(not(target_os = "macos"))]
Expand Down
22 changes: 21 additions & 1 deletion src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ use crate::util::paths;
use crate::util::{self, machine_message, ProcessBuilder};
use crate::util::{internal, join_paths, profile};

/// Indicates whether an object is for the host architcture or the target architecture.
/// Indicates whether an object is for the host architecture or the target architecture.
///
/// These will be the same unless cross-compiling.
#[derive(PartialEq, Eq, Hash, Debug, Clone, Copy, PartialOrd, Ord, Serialize)]
Expand Down Expand Up @@ -915,6 +915,16 @@ fn build_base_args<'a, 'cfg>(
let dir = cx.files().layout(unit.kind).incremental().as_os_str();
opt(cmd, "-C", "incremental=", Some(dir));
}

if unit.is_std {
// -Zforce-unstable-if-unmarked prevents the accidental use of
// unstable crates within the sysroot (such as "extern crate libc" or
// any non-public crate in the sysroot).
//
// RUSTC_BOOTSTRAP allows unstable features on stable.
cmd.arg("-Zforce-unstable-if-unmarked")
.env("RUSTC_BOOTSTRAP", "1");
}
Ok(())
}

Expand Down Expand Up @@ -968,7 +978,17 @@ fn build_deps_args<'a, 'cfg>(

let mut unstable_opts = false;

if let Some(sysroot) = cx.files().layout(Kind::Target).sysroot() {
if unit.kind == Kind::Target {
cmd.arg("--sysroot").arg(sysroot);
}
}

for dep in deps {
if !unit.is_std && dep.unit.is_std {
// Dependency to sysroot crate uses --sysroot.
continue;
}
if dep.unit.mode.is_run_custom_build() {
cmd.env("OUT_DIR", &cx.files().build_script_out_dir(&dep.unit));
}
Expand Down
46 changes: 42 additions & 4 deletions src/cargo/core/compiler/standard_lib.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
//! Code for building the standard library.

use crate::core::compiler::{BuildContext, CompileMode, Kind, Unit};
use super::layout::Layout;
use crate::core::compiler::{BuildContext, CompileMode, Context, FileFlavor, Kind, Unit};
use crate::core::profiles::UnitFor;
use crate::core::resolver::ResolveOpts;
use crate::core::{Dependency, PackageId, PackageSet, Resolve, SourceId, Workspace};
use crate::ops::{self, Packages};
use crate::util::errors::CargoResult;
use crate::util::paths;
use std::collections::{HashMap, HashSet};
use std::env;
use std::path::PathBuf;
Expand Down Expand Up @@ -141,9 +143,15 @@ pub fn generate_std_roots<'a>(
bcx.build_config.release,
);
let features = std_resolve.features_sorted(pkg.package_id());
Ok(bcx
.units
.intern(pkg, lib, profile, Kind::Target, mode, features))
Ok(bcx.units.intern(
pkg,
lib,
profile,
Kind::Target,
mode,
features,
/*is_std*/ true,
))
})
.collect::<CargoResult<Vec<_>>>()
}
Expand Down Expand Up @@ -173,3 +181,33 @@ fn detect_sysroot_src_path(ws: &Workspace<'_>) -> CargoResult<PathBuf> {
}
Ok(src_path)
}

/// Prepare the output directory for the local sysroot.
pub fn prepare_sysroot(layout: &Layout) -> CargoResult<()> {
if let Some(libdir) = layout.sysroot_libdir() {
if libdir.exists() {
paths::remove_dir_all(libdir)?;
}
paths::create_dir_all(libdir)?;
}
Ok(())
}

/// Copy an artifact to the sysroot.
pub fn add_sysroot_artifact<'a>(
cx: &Context<'a, '_>,
unit: &Unit<'a>,
rmeta: bool,
) -> CargoResult<()> {
let outputs = cx.outputs(unit)?;
let outputs = outputs
.iter()
.filter(|output| output.flavor == FileFlavor::Linkable { rmeta })
.map(|output| &output.path);
for path in outputs {
let libdir = cx.files().layout(Kind::Target).sysroot_libdir().unwrap();
let dst = libdir.join(path.file_name().unwrap());
paths::link_or_copy(path, dst)?;
}
Ok(())
}
4 changes: 4 additions & 0 deletions src/cargo/core/compiler/unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ pub struct UnitInner<'a> {
/// The `cfg` features to enable for this unit.
/// This must be sorted.
pub features: Vec<&'a str>,
/// Whether this is a standard library unit.
pub is_std: bool,
}

impl UnitInner<'_> {
Expand Down Expand Up @@ -144,6 +146,7 @@ impl<'a> UnitInterner<'a> {
kind: Kind,
mode: CompileMode,
features: Vec<&'a str>,
is_std: bool,
) -> Unit<'a> {
let inner = self.intern_inner(&UnitInner {
pkg,
Expand All @@ -152,6 +155,7 @@ impl<'a> UnitInterner<'a> {
kind,
mode,
features,
is_std,
});
Unit { inner }
}
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/unit_dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,7 @@ fn new_unit_dep_with_profile<'a>(
let unit = state
.bcx
.units
.intern(pkg, target, profile, kind, mode, features);
.intern(pkg, target, profile, kind, mode, features, state.is_std);
Ok(UnitDep {
unit,
unit_for,
Expand Down
7 changes: 3 additions & 4 deletions src/cargo/ops/cargo_clean.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,9 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> {
)
};
let features = resolve.features_sorted(pkg.package_id());
units.push(
bcx.units
.intern(pkg, target, profile, *kind, *mode, features),
);
units.push(bcx.units.intern(
pkg, target, profile, *kind, *mode, features, /*is_std*/ false,
));
}
}
}
Expand Down
16 changes: 14 additions & 2 deletions src/cargo/ops/cargo_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,11 @@ pub fn compile_ws<'a>(
let (mut packages, resolve_with_overrides) = resolve;

let std_resolve = if let Some(crates) = &config.cli_unstable().build_std {
if build_config.build_plan {
config
.shell()
.warn("-Zbuild-std does not currently fully support --build-plan")?;
}
if build_config.requested_target.is_none() {
// TODO: This should eventually be fixed. Unfortunately it is not
// easy to get the host triple in BuildConfig. Consider changing
Expand Down Expand Up @@ -703,8 +708,15 @@ fn generate_targets<'a>(
bcx.build_config.release,
);
let features = resolve.features_sorted(pkg.package_id());
bcx.units
.intern(pkg, target, profile, kind, target_mode, features)
bcx.units.intern(
pkg,
target,
profile,
kind,
target_mode,
features,
/*is_std*/ false,
)
};

// Create a list of proposed targets.
Expand Down
Loading