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

Don't require dev-dependencies when not needed in certain cases #5012

Merged
merged 9 commits into from
Mar 7, 2018
5 changes: 4 additions & 1 deletion src/bin/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,10 @@ pub fn execute(options: Options, config: &mut Config) -> CliResult {
&options.flag_z)?;

let root = find_root_manifest_for_wd(options.flag_manifest_path, config.cwd())?;
let ws = Workspace::new(&root, config)?;
let mut ws = Workspace::new(&root, config)?;
if config.cli_unstable().avoid_dev_deps {
ws.set_require_optional_deps(false);
}

let spec = Packages::from_flags(options.flag_all,
&options.flag_exclude,
Expand Down
2 changes: 2 additions & 0 deletions src/cargo/core/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@ pub struct CliUnstable {
pub unstable_options: bool,
pub offline: bool,
pub no_index_update: bool,
pub avoid_dev_deps: bool,
}

impl CliUnstable {
Expand Down Expand Up @@ -310,6 +311,7 @@ impl CliUnstable {
"unstable-options" => self.unstable_options = true,
"offline" => self.offline = true,
"no-index-update" => self.no_index_update = true,
"avoid-dev-deps" => self.avoid_dev_deps = true,
_ => bail!("unknown `-Z` flag specified: {}", k),
}

Expand Down
21 changes: 18 additions & 3 deletions src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,14 +98,26 @@ pub struct DepsNotReplaced<'a> {

#[derive(Clone, Copy)]
pub enum Method<'a> {
Everything,
Everything, // equivalent to Required { dev_deps: true, all_features: true, .. }
Required {
dev_deps: bool,
features: &'a [String],
all_features: bool,
uses_default_features: bool,
},
}

impl<'r> Method<'r> {
pub fn split_features(features: &[String]) -> Vec<String> {
features.iter()
.flat_map(|s| s.split_whitespace())
.flat_map(|s| s.split(','))
.filter(|s| !s.is_empty())
.map(|s| s.to_string())
.collect::<Vec<String>>()
}
}

// Information about the dependencies for a crate, a tuple of:
//
// (dependency info, candidates, features activated)
Expand Down Expand Up @@ -906,6 +918,7 @@ fn activate_deps_loop(
let method = Method::Required {
dev_deps: false,
features: &features,
all_features: false,
uses_default_features: dep.uses_default_features(),
};
trace!("{}[{}]>{} trying {}", parent.name(), cur, dep.name(), candidate.summary.version());
Expand Down Expand Up @@ -1249,7 +1262,8 @@ fn build_requirements<'a, 'b: 'a>(s: &'a Summary, method: &'b Method)
-> CargoResult<Requirements<'a>> {
let mut reqs = Requirements::new(s);
match *method {
Method::Everything => {
Method::Everything |
Method::Required { all_features: true, .. } => {
for key in s.features().keys() {
reqs.require_feature(key)?;
}
Expand Down Expand Up @@ -1302,10 +1316,11 @@ impl Context {
}
debug!("checking if {} is already activated", summary.package_id());
let (features, use_default) = match *method {
Method::Everything |
Method::Required { all_features: true, .. } => return Ok(false),
Method::Required { features, uses_default_features, .. } => {
(features, uses_default_features)
}
Method::Everything => return Ok(false),
};

let has_default_feature = summary.features().contains_key("default");
Expand Down
8 changes: 7 additions & 1 deletion src/cargo/core/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ pub struct Workspace<'cfg> {

// True if this workspace should enforce optional dependencies even when
// not needed; false if this workspace should only enforce dependencies
// needed by the current configuration (such as in cargo install).
// needed by the current configuration (such as in cargo install). In some
// cases `false` also results in the non-enforcement of dev-dependencies.
require_optional_deps: bool,
}

Expand Down Expand Up @@ -296,6 +297,11 @@ impl<'cfg> Workspace<'cfg> {
self.require_optional_deps
}

pub fn set_require_optional_deps<'a>(&'a mut self, require_optional_deps: bool) -> &mut Workspace<'cfg> {
self.require_optional_deps = require_optional_deps;
self
}

/// Finds the root of a workspace for the crate whose manifest is located
/// at `manifest_path`.
///
Expand Down
28 changes: 21 additions & 7 deletions src/cargo/ops/cargo_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use std::sync::Arc;

use core::{Source, Package, Target};
use core::{Profile, TargetKind, Profiles, Workspace, PackageId, PackageIdSpec};
use core::resolver::Resolve;
use core::resolver::{Resolve, Method};
use ops::{self, BuildOutput, Executor, DefaultExecutor};
use util::config::Config;
use util::{CargoResult, profile};
Expand Down Expand Up @@ -233,12 +233,18 @@ pub fn compile_ws<'a>(ws: &Workspace<'a>,
let profiles = ws.profiles();

let specs = spec.into_package_id_specs(ws)?;
let resolve = ops::resolve_ws_precisely(ws,
source,
features,
all_features,
no_default_features,
&specs)?;
let features = Method::split_features(features);
let method = Method::Required {
dev_deps: ws.require_optional_deps() || filter.need_dev_deps(),
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps the name optional_deps could change since it's affecting dev-dependencies?

Additionally, I think that we'd want this flag to affect target-specific dependencies, right? If this is a sort of "one shot" compilation there should also be no need to resolve and require windows-specific dependencies when on Linux, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you mind if we leave that as a TODO for the future? What you say makes sense but it is not actually needed in Debian right now, because we union over all targets when translating dependencies - so e.g. winapi etc are all pulled in regardless of what target you're building for. This means we have to have less special-cases in the packaging, and also supports cross-compilation later.

If I understood correctly, on Fedora they are patching out windows dependencies so they don't appear in Cargo.toml, so this functionality wouldn't be needed either. (Perhaps @ignatenkobrain can confirm).

Copy link
Member

Choose a reason for hiding this comment

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

Ok sure yeah, I think a TODO should suffice!

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 unsure if it's appropriate to mention "targets" in the sense of "platform" here on this line. The term "target" in the context of CompileFilter and core::Target and cargo build options, seems rather to mean "lib/bin/examples/tests" etc and not platform.

OTOH the functionality you mention does seem to be missing - if I add [target."XXXX".dependencies] YYY = "999" to Cargo.toml then cargo install still fails with this PR, despite the fact that "XXXX" does not match the current platform. I'm just unsure of the appropriate place to add this TODO, please advise. Perhaps on struct Method?

Copy link
Member

Choose a reason for hiding this comment

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

Hm ok, perhaps an issue can be filed? I think the -Z avoid-dev-deps flag added here may in the long run want to be something like --minimal-cargo-lock which prunes all non-relevant dependencies like platform-specific dependencies that don't apply, dev-deps if you're not building tests, etc. In that sense I think the feature/TODO here is slightly broader than a comment in the code. Would you be ok filing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed as #5133

features: &features,
all_features,
uses_default_features: !no_default_features,
};
let resolve = ops::resolve_ws_with_method(ws,
source,
method,
&specs,
)?;
let (packages, resolve_with_overrides) = resolve;

let to_builds = specs.iter().map(|p| {
Expand Down Expand Up @@ -414,6 +420,14 @@ impl<'a> CompileFilter<'a> {
}
}

pub fn need_dev_deps(&self) -> bool {
match *self {
CompileFilter::Default { .. } => true,
CompileFilter::Only { examples, tests, benches, .. } =>
examples.is_specific() || tests.is_specific() || benches.is_specific()
}
}

pub fn matches(&self, target: &Target) -> bool {
match *self {
CompileFilter::Default { .. } => true,
Expand Down
6 changes: 5 additions & 1 deletion src/cargo/ops/cargo_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,11 @@ fn install_one(root: &Filesystem,

let ws = match overidden_target_dir {
Some(dir) => Workspace::ephemeral(pkg, config, Some(dir), false)?,
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the optional deps flag be set for this 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.

ephemeral() already sets that internally

None => Workspace::new(pkg.manifest_path(), config)?,
None => {
let mut ws = Workspace::new(pkg.manifest_path(), config)?;
ws.set_require_optional_deps(false);
ws
}
};
let pkg = ws.current()?;

Expand Down
2 changes: 1 addition & 1 deletion src/cargo/ops/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ pub use self::registry::{modify_owners, yank, OwnersOptions, PublishOpts};
pub use self::registry::configure_http_handle;
pub use self::cargo_fetch::fetch;
pub use self::cargo_pkgid::pkgid;
pub use self::resolve::{resolve_ws, resolve_ws_precisely, resolve_with_previous};
pub use self::resolve::{resolve_ws, resolve_ws_precisely, resolve_ws_with_method, resolve_with_previous};
pub use self::cargo_output_metadata::{output_metadata, OutputMetadataOptions, ExportInfo};

mod cargo_clean;
Expand Down
35 changes: 19 additions & 16 deletions src/cargo/ops/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,25 @@ pub fn resolve_ws_precisely<'a>(ws: &Workspace<'a>,
no_default_features: bool,
specs: &[PackageIdSpec])
-> CargoResult<(PackageSet<'a>, Resolve)> {
let features = features.iter()
.flat_map(|s| s.split_whitespace())
.flat_map(|s| s.split(','))
.filter(|s| !s.is_empty())
.map(|s| s.to_string())
.collect::<Vec<String>>();
let features = Method::split_features(features);
let method = if all_features {
Method::Everything
} else {
Method::Required {
dev_deps: true,
features: &features,
all_features: false,
uses_default_features: !no_default_features,
}
};
resolve_ws_with_method(ws, source, method, specs)
}

pub fn resolve_ws_with_method<'a>(ws: &Workspace<'a>,
source: Option<Box<Source + 'a>>,
method: Method,
specs: &[PackageIdSpec])
-> CargoResult<(PackageSet<'a>, Resolve)> {
let mut registry = PackageRegistry::new(ws.config())?;
if let Some(source) = source {
registry.add_preloaded(source);
Expand Down Expand Up @@ -68,16 +80,6 @@ pub fn resolve_ws_precisely<'a>(ws: &Workspace<'a>,
ops::load_pkg_lockfile(ws)?
};

let method = if all_features {
Method::Everything
} else {
Method::Required {
dev_deps: true, // TODO: remove this option?
features: &features,
uses_default_features: !no_default_features,
}
};

let resolved_with_overrides =
ops::resolve_with_previous(&mut registry,
ws,
Expand Down Expand Up @@ -236,6 +238,7 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry,
let base = Method::Required {
dev_deps,
features: &[],
all_features: false,
uses_default_features: true,
};
let member_id = member.package_id();
Expand Down
25 changes: 25 additions & 0 deletions tests/testsuite/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4290,3 +4290,28 @@ fn no_linkable_target() {
[WARNING] The package `the_lib` provides no linkable [..] \
while compiling `foo`. [..] in `the_lib`'s Cargo.toml. [..]"));
}

#[test]
fn avoid_dev_deps() {
Package::new("foo", "1.0.0").publish();
let p = project("foo")
.file("Cargo.toml", r#"
[package]
name = "bar"
version = "0.1.0"
authors = []

[dev-dependencies]
baz = "1.0.0"
"#)
.file("src/main.rs", "fn main() {}")
.build();

// --bins is needed because of #5134
assert_that(p.cargo("build").arg("--bins"),
execs().with_status(101));
assert_that(p.cargo("build").arg("--bins")
.masquerade_as_nightly_cargo()
.arg("-Zavoid-dev-deps"),
execs().with_status(0));
}
57 changes: 57 additions & 0 deletions tests/testsuite/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::fs::{self, File, OpenOptions};
use std::io::prelude::*;

use cargo::util::ProcessBuilder;
use cargotest::ChannelChanger;
use cargotest::install::{cargo_home, has_installed_exe};
use cargotest::support::git;
use cargotest::support::paths;
Expand Down Expand Up @@ -904,6 +905,62 @@ fn use_path_workspace() {
assert_eq!(lock, lock2, "different lockfiles");
}

#[test]
fn dev_dependencies_no_check() {
Package::new("foo", "1.0.0").publish();
let p = project("foo")
.file("Cargo.toml", r#"
[package]
name = "bar"
version = "0.1.0"
authors = []

[dev-dependencies]
baz = "1.0.0"
"#)
.file("src/main.rs", "fn main() {}")
.build();

// --bins is needed because of #5134
assert_that(p.cargo("build").arg("--bins"),
execs().with_status(101));
assert_that(p.cargo("install").arg("--bins"),
execs().with_status(0));
}

#[test]
fn dev_dependencies_lock_file_untouched() {
Package::new("foo", "1.0.0").publish();
let p = project("foo")
.file("Cargo.toml", r#"
[package]
name = "foo"
version = "0.1.0"
authors = []

[dev-dependencies]
bar = { path = "a" }
"#)
.file("src/main.rs", "fn main() {}")
.file("a/Cargo.toml", r#"
[package]
name = "bar"
version = "0.1.0"
authors = []
"#)
.file("a/src/lib.rs", "")
.build();

// --bins is needed because of #5134
assert_that(p.cargo("build").arg("--bins"),
execs().with_status(0));
let lock = p.read_lockfile();
assert_that(p.cargo("install").arg("--bins"),
execs().with_status(0));
let lock2 = p.read_lockfile();
assert!(lock == lock2, "different lockfiles");
}

#[test]
fn vers_precise() {
pkg("foo", "0.1.1");
Expand Down