Skip to content
Merged
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
8 changes: 4 additions & 4 deletions src/cargo/core/compiler/context/compilation_files.rs
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
crate_type,
flavor,
unit.target.kind(),
cx.target_triple(),
cx.build_config.target_triple(),
)?;

match file_types {
Expand Down Expand Up @@ -321,14 +321,14 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
does not support these crate types",
unsupported.join(", "),
unit.pkg,
cx.target_triple()
cx.build_config.target_triple()
)
}
bail!(
"cannot compile `{}` as the target `{}` does not \
support any of the output crate types",
unit.pkg,
cx.target_triple()
cx.build_config.target_triple()
);
}
info!("Target filenames: {:?}", ret);
Expand Down Expand Up @@ -380,7 +380,7 @@ fn compute_metadata<'a, 'cfg>(
let __cargo_default_lib_metadata = env::var("__CARGO_DEFAULT_LIB_METADATA");
if !(unit.profile.test || unit.profile.check)
&& (unit.target.is_dylib() || unit.target.is_cdylib()
|| (unit.target.is_bin() && cx.target_triple().starts_with("wasm32-")))
|| (unit.target.is_bin() && cx.build_config.target_triple().starts_with("wasm32-")))
&& unit.pkg.package_id().source_id().is_path()
&& __cargo_default_lib_metadata.is_err()
{
Expand Down
68 changes: 25 additions & 43 deletions src/cargo/core/compiler/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,13 +125,31 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
None => Client::new(build_config.jobs as usize - 1)
.chain_err(|| "failed to create jobserver")?,
};

let (host_info, target_info) = {
let _p = profile::start("Context::probe_target_info");
debug!("probe_target_info");
let host_target_same = match build_config.requested_target {
Some(ref s) if s != &config.rustc()?.host => false,
_ => true,
};

let host_info = TargetInfo::new(config, &build_config, Kind::Host)?;
let target_info = if host_target_same {
host_info.clone()
} else {
TargetInfo::new(config, &build_config, Kind::Target)?
};
(host_info, target_info)
};

let mut cx = Context {
ws,
resolve,
packages,
config,
target_info: TargetInfo::default(),
host_info: TargetInfo::default(),
target_info,
host_info,
compilation: Compilation::new(config),
build_state: Arc::new(BuildState::new(&build_config)),
build_config,
Expand All @@ -150,7 +168,8 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
files: None,
};

cx.probe_target_info()?;
cx.compilation.host_dylib_path = cx.host_info.sysroot_libdir.clone();
cx.compilation.target_dylib_path = cx.target_info.sysroot_libdir.clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move this into Compilation::new as well?

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 definitely thought about it. I decided against it for now mostly on style (passing around long names is kind of ugly). I just looked into how the rest of Compilation gets initialized, and it looks like pretty much everything gets initialized after the fact from throughout Context, so passing these two in at new() is not a win for consistency. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's leave it as is

Ok(cx)
}

Expand Down Expand Up @@ -283,7 +302,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
self.compilation.native_dirs.insert(dir.clone());
}
}
self.compilation.target = self.target_triple().to_string();
self.compilation.target = self.build_config.target_triple().to_string();
Ok(self.compilation)
}

Expand Down Expand Up @@ -341,27 +360,6 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
Ok(())
}

/// Ensure that we've collected all target-specific information to compile
/// all the units mentioned in `units`.
fn probe_target_info(&mut self) -> CargoResult<()> {
let _p = profile::start("Context::probe_target_info");
debug!("probe_target_info");
let host_target_same = match self.requested_target() {
Some(s) if s != self.config.rustc()?.host => false,
_ => true,
};

self.host_info = TargetInfo::new(self, Kind::Host)?;
self.target_info = if host_target_same {
self.host_info.clone()
} else {
TargetInfo::new(self, Kind::Target)?
};
self.compilation.host_dylib_path = self.host_info.sysroot_libdir.clone();
self.compilation.target_dylib_path = self.target_info.sysroot_libdir.clone();
Ok(())
}

/// Builds up the `used_in_plugin` internal to this context from the list of
/// top-level units.
///
Expand Down Expand Up @@ -401,22 +399,6 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
self.files.as_mut().unwrap()
}

/// Return the host triple for this context
pub fn host_triple(&self) -> &str {
&self.build_config.host_triple
}

/// Return the target triple which this context is targeting.
pub fn target_triple(&self) -> &str {
self.requested_target()
.unwrap_or_else(|| self.host_triple())
}

/// Requested (not actual) target for the build
pub fn requested_target(&self) -> Option<&str> {
self.build_config.requested_target.as_ref().map(|s| &s[..])
}

/// Return the filenames that the given target for the given profile will
/// generate as a list of 3-tuples (filename, link_dst, linkable)
///
Expand Down Expand Up @@ -458,8 +440,8 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
None => return true,
};
let (name, info) = match kind {
Kind::Host => (self.host_triple(), &self.host_info),
Kind::Target => (self.target_triple(), &self.target_info),
Kind::Host => (self.build_config.host_triple.as_ref(), &self.host_info),
Kind::Target => (self.build_config.target_triple(), &self.target_info),
};
platform.matches(name, info.cfg())
}
Expand Down
16 changes: 8 additions & 8 deletions src/cargo/core/compiler/context/target_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ use std::collections::hash_map::{Entry, HashMap};
use std::path::PathBuf;
use std::str::{self, FromStr};

use super::{env_args, Context};
use util::{CargoResult, CargoResultExt, Cfg, ProcessBuilder};
use super::{env_args, BuildConfig};
use util::{CargoResult, CargoResultExt, Cfg, Config, ProcessBuilder};
use core::TargetKind;
use super::Kind;

#[derive(Clone, Default)]
#[derive(Clone)]
pub struct TargetInfo {
crate_type_process: Option<ProcessBuilder>,
crate_types: RefCell<HashMap<String, Option<(String, String)>>>,
Expand Down Expand Up @@ -51,9 +51,9 @@ impl FileType {
}

impl TargetInfo {
pub fn new(cx: &Context, kind: Kind) -> CargoResult<TargetInfo> {
let rustflags = env_args(cx.config, &cx.build_config, None, kind, "RUSTFLAGS")?;
let mut process = cx.config.rustc()?.process();
pub fn new(config: &Config, build_config: &BuildConfig, kind: Kind) -> CargoResult<TargetInfo> {
let rustflags = env_args(config, build_config, None, kind, "RUSTFLAGS")?;
let mut process = config.rustc()?.process();
process
.arg("-")
.arg("--crate-name")
Expand All @@ -63,7 +63,7 @@ impl TargetInfo {
.env_remove("RUST_LOG");

if kind == Kind::Target {
process.arg("--target").arg(&cx.target_triple());
process.arg("--target").arg(&build_config.target_triple());
}

let crate_type_process = process.clone();
Expand Down Expand Up @@ -115,7 +115,7 @@ impl TargetInfo {
} else {
rustlib.push("lib");
rustlib.push("rustlib");
rustlib.push(cx.target_triple());
rustlib.push(build_config.target_triple());
rustlib.push("lib");
sysroot_libdir = Some(rustlib);
}
Expand Down
6 changes: 3 additions & 3 deletions src/cargo/core/compiler/custom_build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,8 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes
.env(
"TARGET",
&match unit.kind {
Kind::Host => cx.host_triple(),
Kind::Target => cx.target_triple(),
Kind::Host => &cx.build_config.host_triple,
Kind::Target => cx.build_config.target_triple(),
},
)
.env("DEBUG", &profile.debuginfo.is_some().to_string())
Expand All @@ -141,7 +141,7 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes
"debug"
},
)
.env("HOST", cx.host_triple())
.env("HOST", &cx.build_config.host_triple)
.env("RUSTC", &cx.config.rustc()?.path)
.env("RUSTDOC", &*cx.config.rustdoc()?)
.inherit_jobserver(&cx.jobserver);
Expand Down
13 changes: 11 additions & 2 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,12 @@ impl BuildConfig {
..Default::default()
})
}

pub fn target_triple(&self) -> &str {
self.requested_target
.as_ref()
.unwrap_or_else(|| &self.host_triple)
}
}

/// Information required to build for a target
Expand Down Expand Up @@ -601,7 +607,7 @@ fn rustdoc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoResult
add_path_args(cx, unit, &mut rustdoc);

if unit.kind != Kind::Host {
if let Some(target) = cx.requested_target() {
if let Some(ref target) = cx.build_config.requested_target {
rustdoc.arg("--target").arg(target);
}
}
Expand Down Expand Up @@ -862,7 +868,10 @@ fn build_base_args<'a, 'cfg>(
cmd,
"--target",
"",
cx.requested_target().map(|s| s.as_ref()),
cx.build_config
.requested_target
.as_ref()
.map(|s| s.as_ref()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make some of this fields into methods of build_config, so that the callers need not to worry about moving out of it and turning an &Option<String> into Option<&str>? Could we make some of BuildConfig fields private?

Copy link
Contributor Author

@djc djc Apr 13, 2018

Choose a reason for hiding this comment

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

For requested_target, sure, I think such a method could make sense. As for making the fields private, I'm not really sure what the point would be? It looks like it was defined very much just as a bag of properties, and the stuff in there is straightforward enough (and typed in such a way) that there doesn't seem to be much downside to just exposing them like they are now. (I should mention that my previous language of choice was Python, where the "consenting adults" philosophy very much reigned supreme, so my take might be more controversial if you're coming from C++ or Java.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no strong opinions about this, but to me "using only fields" or "using only methods" seems slightly superior to "use fields for some stuff and methods for some other stuff",

Code like

                Kind::Host => &cx.build_config.host_triple,
                Kind::Target => cx.build_config.target_triple(),

seems slightly funny, and makes you pause for a split second.

And looks like after #5354 we might need less public access to fields?

But, as I've said, it's an ultra minor issue :)

Copy link
Contributor Author

@djc djc Apr 13, 2018

Choose a reason for hiding this comment

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

If you want to go all or none I would leave it as none. :) After replacing all instances of .requested_target with a method .requested_target() (a diff which in itself is +13/-13), there are 38 instances of build_config\., about 14 of which are method calls, so that's a slight minority.

);
}

Expand Down
12 changes: 8 additions & 4 deletions src/cargo/ops/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,6 @@ pub fn resolve_with_previous<'a, 'cfg>(
registry.lock_patches();
}


for member in ws.members() {
registry.add_sources(&[member.package_id().source_id().clone()])?;
}
Expand All @@ -223,12 +222,18 @@ pub fn resolve_with_previous<'a, 'cfg>(
let mut members = Vec::new();
match method {
Method::Everything => members.extend(ws.members()),
Method::Required { features, all_features, uses_default_features, .. } => {
Method::Required {
features,
all_features,
uses_default_features,
..
} => {
if specs.len() > 1 && !features.is_empty() {
bail!("cannot specify features for more than one package");
}
members.extend(
ws.members().filter(|m| specs.iter().any(|spec| spec.matches(m.package_id())))
ws.members()
.filter(|m| specs.iter().any(|spec| spec.matches(m.package_id()))),
);
// Edge case: running `cargo build -p foo`, where `foo` is not a member
// of current workspace. Add all packages from workspace to get `foo`
Expand Down Expand Up @@ -293,7 +298,6 @@ pub fn resolve_with_previous<'a, 'cfg>(
}
};


let root_replace = ws.root_replace();

let replace = match previous {
Expand Down
27 changes: 17 additions & 10 deletions tests/testsuite/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1664,39 +1664,46 @@ fn combining_features_and_package() {
main = []
"#,
)
.file("foo/src/main.rs", r#"
.file(
"foo/src/main.rs",
r#"
#[cfg(feature = "main")]
fn main() {}
"#)
"#,
)
.build();

assert_that(
p.cargo("build -Z package-features --all --features main")
.masquerade_as_nightly_cargo(),
execs().with_status(101).with_stderr_contains("\
[ERROR] cannot specify features for more than one package"
execs().with_status(101).with_stderr_contains(
"\
[ERROR] cannot specify features for more than one package",
),
);

assert_that(
p.cargo("build -Z package-features --package dep --features main")
.masquerade_as_nightly_cargo(),
execs().with_status(101).with_stderr_contains("\
[ERROR] cannot specify features for packages outside of workspace"
execs().with_status(101).with_stderr_contains(
"\
[ERROR] cannot specify features for packages outside of workspace",
),
);
assert_that(
p.cargo("build -Z package-features --package dep --all-features")
.masquerade_as_nightly_cargo(),
execs().with_status(101).with_stderr_contains("\
[ERROR] cannot specify features for packages outside of workspace"
execs().with_status(101).with_stderr_contains(
"\
[ERROR] cannot specify features for packages outside of workspace",
),
);
assert_that(
p.cargo("build -Z package-features --package dep --no-default-features")
.masquerade_as_nightly_cargo(),
execs().with_status(101).with_stderr_contains("\
[ERROR] cannot specify features for packages outside of workspace"
execs().with_status(101).with_stderr_contains(
"\
[ERROR] cannot specify features for packages outside of workspace",
),
);

Expand Down