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

Stricter need_dev_deps behaviour #5186

Merged
merged 6 commits into from
Mar 21, 2018
Merged

Stricter need_dev_deps behaviour #5186

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3e07a3e
Fix a bug in #5152 that causes rustc/rustdoc to fail unnecessarily
infinity0 Mar 14, 2018
89d2748
Revert "Work around #5134 for now"
infinity0 Mar 15, 2018
89d5161
Fix need_dev_deps to return false during a default `cargo build` run
infinity0 Mar 15, 2018
0bf8e54
Add tests for --all-targets
infinity0 Mar 15, 2018
ce26ddf
Rename stuff for clarity
infinity0 Mar 15, 2018
0bc1155
Split tests, apparently `cargo clean` does not work well on windows
infinity0 Mar 19, 2018
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
7 changes: 6 additions & 1 deletion src/bin/commands/rustc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,12 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
}
};
let mut compile_opts = args.compile_options_for_single_package(config, mode)?;
compile_opts.target_rustc_args = Some(values(args, "args"));
let target_args = values(args, "args");
compile_opts.target_rustc_args = if target_args.is_empty() {
Copy link
Member

Choose a reason for hiding this comment

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

Was this change needed for the correctness of the patch? I think it changes the behavior of cargo rustc by default (erroring today whereas after this patch it succeeds and builds multiple crates)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is needed for e.g. cargo rustc with no args to work in the first place.

Assuming "package" and "crate" are the same thing, the behaviour you're describing is tested for by fail_with_multiple_packages which still passes. There is also a fails_when_trying_to_build_main_and_lib_with_args test for failing when multiple targets are selected, and it is still passing 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.

Yes, this is needed for e.g. cargo rustc with no args to work in the first place.

Sorry, this is not correct. The hunk here is needed for cargo rustc --all-targets with no further args to work, which is documented as part of its CLI. Furthermore it's also needed when passing cargo args like -v, as opposed to binary-target args that go after -- which is what fails_when_trying_to_build_main_and_lib_with_args tests for and this is still passing.

Copy link
Member

Choose a reason for hiding this comment

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

Hm I think the documentation is wrong then? The cargo rustc command was only ever intended to work with only one target

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok but my commit here doesn't affect this (i.e. it was broken before and it's broken afterwards), it only fixes the -- related behaviour that was broken by #5152. If you run cargo rustc using cargo 0.25.0 (the current stable) then it will still work with a multi-target crate, e.g.

$ cat Cargo.toml 
[package]
name = "foo"
version = "0.0.1"
authors = []

$ cat src/lib.rs 
pub fn myfunc() {}

$ cat src/main.rs 
fn main () {}

$ cargo --version
cargo 0.25.0

$ cargo rustc --verbose
   Compiling foo v0.0.1 (file://$PWD)
     Running `rustc --crate-name foo src/lib.rs --crate-type lib --emit=dep-info,link -C debuginfo=2 -C metadata=0fa9827c683f047b -C extra-filename=-0fa9827c683f047b --out-dir $PWD/target/debug/deps -L dependency=$PWD/target/debug/deps`
     Running `rustc --crate-name foo src/main.rs --crate-type bin --emit=dep-info,link -C debuginfo=2 -C metadata=a9125b8469059127 -C extra-filename=-a9125b8469059127 --out-dir $PWD/target/debug/deps -L dependency=$PWD/target/debug/deps --extern foo=$PWD/target/debug/deps/libfoo-0fa9827c683f047b.rlib`
    Finished dev [unoptimized + debuginfo] target(s) in 0.35 secs

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... odd! Well in any case it looks like nothing's changing so seems good to land!

None
} else {
Some(target_args)
};
ops::compile(&ws, &compile_opts)?;
Ok(())
}
7 changes: 6 additions & 1 deletion src/bin/commands/rustdoc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,12 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
let ws = args.workspace(config)?;
let mut compile_opts =
args.compile_options_for_single_package(config, CompileMode::Doc { deps: false })?;
compile_opts.target_rustdoc_args = Some(values(args, "args"));
let target_args = values(args, "args");
compile_opts.target_rustdoc_args = if target_args.is_empty() {
None
} else {
Some(target_args)
};
let doc_opts = DocOptions {
open_result: args.is_present("open"),
compile_opts,
Expand Down
32 changes: 19 additions & 13 deletions src/cargo/ops/cargo_compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ pub fn compile_ws<'a>(
let specs = spec.into_package_id_specs(ws)?;
let features = Method::split_features(features);
let method = Method::Required {
dev_deps: ws.require_optional_deps() || filter.need_dev_deps(),
dev_deps: ws.require_optional_deps() || filter.need_dev_deps(mode),
features: &features,
all_features,
uses_default_features: !no_default_features,
Expand Down Expand Up @@ -442,19 +442,25 @@ impl CompileFilter {
}
}

pub fn need_dev_deps(&self) -> bool {
match *self {
CompileFilter::Default { .. } => true,
CompileFilter::Only {
ref examples,
ref tests,
ref benches,
..
} => examples.is_specific() || tests.is_specific() || benches.is_specific(),
pub fn need_dev_deps(&self, mode: CompileMode) -> bool {
match mode {
CompileMode::Test | CompileMode::Doctest | CompileMode::Bench => true,
CompileMode::Build | CompileMode::Doc { .. } | CompileMode::Check { .. } => match *self
{
CompileFilter::Default { .. } => false,
CompileFilter::Only {
ref examples,
ref tests,
ref benches,
..
} => examples.is_specific() || tests.is_specific() || benches.is_specific(),
},
}
}

pub fn matches(&self, target: &Target) -> bool {
// this selects targets for "cargo run". for logic to select targets for
// other subcommands, see generate_targets and generate_default_targets
pub fn target_run(&self, target: &Target) -> bool {
match *self {
CompileFilter::Default { .. } => true,
CompileFilter::Only {
Expand Down Expand Up @@ -493,7 +499,7 @@ struct BuildProposal<'a> {
required: bool,
}

fn generate_auto_targets<'a>(
fn generate_default_targets<'a>(
mode: CompileMode,
targets: &'a [Target],
profile: &'a Profile,
Expand Down Expand Up @@ -715,7 +721,7 @@ fn generate_targets<'a>(
} else {
&profiles.test_deps
};
generate_auto_targets(
generate_default_targets(
mode,
pkg.targets(),
profile,
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/ops/cargo_run.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ pub fn run(
!a.is_lib() && !a.is_custom_build() && if !options.filter.is_specific() {
a.is_bin()
} else {
options.filter.matches(a)
options.filter.target_run(a)
}
})
.map(|bin| bin.name())
Expand Down
53 changes: 50 additions & 3 deletions tests/testsuite/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5336,6 +5336,55 @@ fn build_filter_infer_profile() {
);
}

#[test]
fn all_targets_with_and_without() {
let p = project("foo")
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"
authors = []
"#,
)
.file("src/main.rs", "fn main() {}")
.build();
assert_that(
p.cargo("build").arg("-v").arg("--all-targets"),
execs().with_status(0)
// bin
.with_stderr_contains("\
[RUNNING] `rustc --crate-name foo src[/]main.rs --crate-type bin \
--emit=dep-info,link[..]")
// bench
.with_stderr_contains("\
[RUNNING] `rustc --crate-name foo src[/]main.rs --emit=dep-info,link \
-C opt-level=3 --test [..]")
// unit test
.with_stderr_contains("\
[RUNNING] `rustc --crate-name foo src[/]main.rs --emit=dep-info,link \
-C debuginfo=2 --test [..]"),
);
assert_that(p.cargo("clean"), execs().with_status(0));
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately we've had pretty bad luck with commands like this which delete the target directory (intermittent failures on windows due to presumed mspdbsrv.exe instances), in light of that could these tests be extracted to individual tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

assert_that(
p.cargo("build").arg("-v"),
execs().with_status(0)
// bin
.with_stderr_contains("\
[RUNNING] `rustc --crate-name foo src[/]main.rs --crate-type bin \
--emit=dep-info,link[..]")
// bench
.with_stderr_does_not_contain("\
[RUNNING] `rustc --crate-name foo src[/]main.rs --emit=dep-info,link \
-C opt-level=3 --test [..]")
// unit test
.with_stderr_does_not_contain("\
[RUNNING] `rustc --crate-name foo src[/]main.rs --emit=dep-info,link \
-C debuginfo=2 --test [..]"),
);
}

#[test]
fn all_targets_no_lib() {
let p = project("foo")
Expand Down Expand Up @@ -5426,11 +5475,9 @@ fn avoid_dev_deps() {
.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"), execs().with_status(101));
assert_that(
p.cargo("build")
.arg("--bins")
.masquerade_as_nightly_cargo()
.arg("-Zavoid-dev-deps"),
execs().with_status(0),
Expand Down
13 changes: 12 additions & 1 deletion tests/testsuite/check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,7 @@ fn check_virtual_all_implied() {
}

#[test]
fn check_all_targets() {
fn all_targets_with_and_without() {
let foo = project("foo")
.file("Cargo.toml", SIMPLE_MANIFEST)
.file("src/main.rs", "fn main() {}")
Expand All @@ -626,6 +626,17 @@ fn check_all_targets() {
.with_stderr_contains("[..] --crate-name test2 tests[/]test2.rs [..]")
.with_stderr_contains("[..] --crate-name bench3 benches[/]bench3.rs [..]"),
);
assert_that(foo.cargo("clean"), execs().with_status(0));
assert_that(
foo.cargo("check").arg("-v"),
execs()
.with_status(0)
.with_stderr_contains("[..] --crate-name foo src[/]lib.rs [..]")
.with_stderr_contains("[..] --crate-name foo src[/]main.rs [..]")
.with_stderr_does_not_contain("[..] --crate-name example1 examples[/]example1.rs [..]")
.with_stderr_does_not_contain("[..] --crate-name test2 tests[/]test2.rs [..]")
.with_stderr_does_not_contain("[..] --crate-name bench3 benches[/]bench3.rs [..]"),
);
}

#[test]
Expand Down
10 changes: 4 additions & 6 deletions tests/testsuite/install.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1222,9 +1222,8 @@ fn dev_dependencies_no_check() {
.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));
assert_that(p.cargo("build"), execs().with_status(101));
assert_that(p.cargo("install"), execs().with_status(0));
}

#[test]
Expand Down Expand Up @@ -1256,10 +1255,9 @@ fn dev_dependencies_lock_file_untouched() {
.file("a/src/lib.rs", "")
.build();

// --bins is needed because of #5134
assert_that(p.cargo("build").arg("--bins"), execs().with_status(0));
assert_that(p.cargo("build"), execs().with_status(0));
let lock = p.read_lockfile();
assert_that(p.cargo("install").arg("--bins"), execs().with_status(0));
assert_that(p.cargo("install"), execs().with_status(0));
let lock2 = p.read_lockfile();
assert!(lock == lock2, "different lockfiles");
}
Expand Down
49 changes: 49 additions & 0 deletions tests/testsuite/rustc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,55 @@ fn build_only_bar_dependency() {
);
}

#[test]
fn all_targets_with_and_without() {
let p = project("foo")
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"
authors = []
"#,
)
.file("src/main.rs", "fn main() {}")
.build();
assert_that(
p.cargo("rustc").arg("-v").arg("--all-targets"),
execs().with_status(0)
// bin
.with_stderr_contains("\
[RUNNING] `rustc --crate-name foo src[/]main.rs --crate-type bin \
--emit=dep-info,link[..]")
// bench
.with_stderr_contains("\
[RUNNING] `rustc --crate-name foo src[/]main.rs --emit=dep-info,link \
-C opt-level=3 --test [..]")
// unit test
.with_stderr_contains("\
[RUNNING] `rustc --crate-name foo src[/]main.rs --emit=dep-info,link \
-C debuginfo=2 --test [..]"),
);
assert_that(p.cargo("clean"), execs().with_status(0));
assert_that(
p.cargo("rustc").arg("-v"),
execs().with_status(0)
// bin
.with_stderr_contains("\
[RUNNING] `rustc --crate-name foo src[/]main.rs --crate-type bin \
--emit=dep-info,link[..]")
// bench
.with_stderr_does_not_contain("\
[RUNNING] `rustc --crate-name foo src[/]main.rs --emit=dep-info,link \
-C opt-level=3 --test [..]")
// unit test
.with_stderr_does_not_contain("\
[RUNNING] `rustc --crate-name foo src[/]main.rs --emit=dep-info,link \
-C debuginfo=2 --test [..]"),
);
}

#[test]
fn fail_with_multiple_packages() {
let foo = project("foo")
Expand Down