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

infinity0 · 2018-02-05T17:16:09Z

Specifically, when running cargo install and add a flag for this behaviour in cargo build.

This closes #4988.

rust-highfive · 2018-02-05T17:16:20Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

…eps`

cargo build --help says: --all-targets Build all targets (lib and bin targets by default) but the old default behaviour was actually doing --all-targets. This meant that --avoid-dev-deps was only effectively if one gave --lib --bins explicitly. With this commit, `cargo build --avoid-dev-deps` with no other flags, will build lib and bins and avoid installing dev-dependencies.

alexcrichton

Thanks! Would it also be possible to include tests as well for this new functionality?

alexcrichton · 2018-02-07T19:27:23Z

src/bin/build.rs

@@ -63,6 +64,7 @@ Options:
    --features FEATURES          Space-separated list of features to also build
    --all-features               Build all available features
    --no-default-features        Do not build the `default` feature
+    --avoid-dev-deps             Avoid installing dev-dependencies if possible


Could this start out as a -Z flag to be unstable by default?

alexcrichton · 2018-02-07T19:28:16Z

src/cargo/ops/cargo_compile.rs

+                TargetKind::Bin => true,
+                TargetKind::Lib(..) => true,
+                _ => false,
+            },


How come this needed changing?

It's not necessary and could be dropped from this PR. But without it, the current situation means I have to give extra flags to cargo build, it contradicts the documentation, and it also means the all-targets flag is redundant.

Hm sorry I missed this earlier, but I'm not sure I quite understand this. Could this perhaps be left out for a future PR?

(or maybe a test could be added?)

This actually opened up a whole new can of worms, so I've reverted it from this PR and opened up a new issue #5134 which I'll file a PR for as soon as this PR is merged (I have a fix, but it builds on top of this PR).

alexcrichton · 2018-02-07T19:29:01Z

src/cargo/ops/cargo_install.rs

@@ -175,7 +175,11 @@ fn install_one(root: &Filesystem,

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


Shouldn't the optional deps flag be set for this as well?

ephemeral() already sets that internally

alexcrichton · 2018-02-07T19:30:23Z

src/cargo/ops/cargo_compile.rs

-                                            &specs)?;
+    let features = Method::split_features(features);
+    let method = Method::Required {
+        dev_deps: ws.require_optional_deps() || filter.need_dev_deps(),


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?

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).

Ok sure yeah, I think a TODO should suffice!

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?

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?

Filed as #5133

infinity0 · 2018-02-09T14:17:22Z

Thanks for the comments, I'll soon find some time to write tests and switch to -Z for the new flag.

infinity0 · 2018-02-16T15:09:45Z

Hey, I've added some tests and changed avoid-dev-deps to be a -Z unstable flag.

I wasn't quite sure what to rename require_optional_deps to, and it's a public struct so switching would carry some extra cost. I did add a comment to it, describing the new behaviour. I could change it if you want though, like require_extra_deps or require_unneeded_deps, let me know which you prefer.

bors · 2018-02-21T20:20:44Z

☔ The latest upstream changes (presumably #5063) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2018-02-26T15:33:49Z

Thanks for the update, tests look good to me! It's also ok to not worry much about naming, despite being public we bump the major version for Cargo regularly so it's not too critical to think too hard about APIs

…tation" This reverts commit 3fc0715.

infinity0 · 2018-03-06T20:03:21Z

I've resolved the merge conflict and dealt with all the remaining issues I think, please take a look and merge if good.

alexcrichton · 2018-03-06T22:09:20Z

Ah looks like some tests may be failing?

infinity0 · 2018-03-07T14:15:38Z

Turns out the tests were also dependent on the fix for #5134, I've pushed a commit to work around it. Should be all good now.

alexcrichton · 2018-03-07T15:39:21Z

@bors: r+

bors · 2018-03-07T15:39:22Z

📌 Commit d46db71 has been approved by alexcrichton

bors · 2018-03-07T15:39:29Z

⌛ Testing commit d46db71 with merge 879f483...

Don't require dev-dependencies when not needed in certain cases Specifically, when running `cargo install` and add a flag for this behaviour in `cargo build`. This closes #4988.

bors · 2018-03-07T16:13:36Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 879f483 to master...

Stricter need_dev_deps behaviour The previous PR (#5012) contained an unnecessary work-around for behaviour of `--all-targets` that was misunderstood. This PR removes that work-around and adds some tests and comments to clarify the behaviour for future contributors, which may help to make easier a future fix for #5177 and #5178.

Applied-Upstream: rust-lang/cargo#5012 Applied-Upstream: rust-lang/cargo#5186 Gbp-Pq: Name 1001_PR5012.patch

This fixes an accidental regression introduced in rust-lang#5012 where the `--all-features` CLI flag was only propagated to the "main crate" as opposed to all workspace packages. This behavior has [already been deemed][pr] as "basically not what you want", but for now it's best to avoid the regression. Closes rust-lang#5518 [pr]: rust-lang#5353

Copy `--all-features` request to all workspace members This fixes an accidental regression introduced in #5012 where the `--all-features` CLI flag was only propagated to the "main crate" as opposed to all workspace packages. This behavior has [already been deemed][pr] as "basically not what you want", but for now it's best to avoid the regression. Closes #5518 [pr]: #5353

This fixes an accidental regression introduced in rust-lang#5012 where the `--all-features` CLI flag was only propagated to the "main crate" as opposed to all workspace packages. This behavior has [already been deemed][pr] as "basically not what you want", but for now it's best to avoid the regression. Closes rust-lang#5518 [pr]: rust-lang#5353

Applied-Upstream: rust-lang/cargo#5012 Applied-Upstream: rust-lang/cargo#5186 Gbp-Pq: Name 1001_PR5012.patch

rust-highfive assigned alexcrichton Feb 5, 2018

infinity0 added 3 commits February 6, 2018 01:35

Don't check for dev-dependencies when not needed

7de30dd

Only avoid dev deps in cargo install and `cargo build --avoid-dev-d…

ee78456

…eps`

infinity0 force-pushed the pr4988 branch from 56c613a to 3fc0715 Compare February 6, 2018 00:35

alexcrichton reviewed Feb 7, 2018

View reviewed changes

infinity0 added 2 commits February 15, 2018 15:31

Turn --avoid-dev-deps into a -Z unstable flag

df5f7d6

Add some tests to check that dev-dependencies are not resolved

da41b4e

infinity0 added 3 commits March 3, 2018 12:56

Merge remote-tracking branch 'upstream/master' into pr4988

9c5eecd

Merge remote-tracking branch 'upstream/master' into pr4988

31f6f84

Revert "Make the default behaviour of cargo build match the documen…

70170d1

…tation" This reverts commit 3fc0715.

infinity0 mentioned this pull request Mar 6, 2018

Stabilize -Z avoid-dev-deps as --minimal-cargo-lock #5133

Open

Work around rust-lang#5134 for now

d46db71

bors merged commit d46db71 into rust-lang:master Mar 7, 2018

infinity0 mentioned this pull request Mar 15, 2018

Stricter need_dev_deps behaviour #5186

Merged

raspbian-autopush pushed a commit to raspbian-packages/cargo that referenced this pull request Mar 28, 2018

Avoid installing dev-dependencies

6daaf27

Applied-Upstream: rust-lang/cargo#5012 Applied-Upstream: rust-lang/cargo#5186 Gbp-Pq: Name 1001_PR5012.patch

This was referenced May 21, 2018

Copy --all-features request to all workspace members #5556

Merged

cargo build --all --all-features in a non-virtual workspace ignores features not present in the root crate #5518

Closed

raspbian-autopush pushed a commit to raspbian-packages/cargo that referenced this pull request Jun 20, 2018

Avoid installing dev-dependencies

3c35847

Applied-Upstream: rust-lang/cargo#5012 Applied-Upstream: rust-lang/cargo#5186 Gbp-Pq: Name 1001_PR5012.patch

raspbian-autopush pushed a commit to raspbian-packages/cargo that referenced this pull request Jun 20, 2018

Avoid installing dev-dependencies

cc0cb66

Applied-Upstream: rust-lang/cargo#5012 Applied-Upstream: rust-lang/cargo#5186 Gbp-Pq: Name 1001_PR5012.patch

polyzen mentioned this pull request Nov 19, 2018

error: failed to run custom build command for openssl v0.9.24 scottlamb/moonfire-nvr#60

Closed

qryxip mentioned this pull request Sep 20, 2020

Run cargo install on the CI tanakh/cargo-atcoder#45

Open

ehuss added this to the 1.26.0 milestone Feb 6, 2022

betarelease mentioned this pull request Feb 24, 2022

#373 Adds ability to automate developer tasks pyrsia/pyrsia#393

Closed

10 tasks

jakoschiko mentioned this pull request Sep 2, 2024

Migrate benches to divan JesperAxelsson/rust-intmap#64

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

infinity0 commented Feb 5, 2018

rust-highfive commented Feb 5, 2018

alexcrichton left a comment

alexcrichton Feb 7, 2018

alexcrichton Feb 7, 2018

infinity0 Feb 9, 2018

alexcrichton Feb 26, 2018

alexcrichton Feb 26, 2018

infinity0 Mar 6, 2018

alexcrichton Feb 7, 2018

infinity0 Feb 9, 2018

alexcrichton Feb 7, 2018

infinity0 Feb 9, 2018

alexcrichton Feb 9, 2018

infinity0 Feb 16, 2018

alexcrichton Feb 26, 2018

infinity0 Mar 6, 2018

infinity0 commented Feb 9, 2018

infinity0 commented Feb 16, 2018

bors commented Feb 21, 2018

alexcrichton commented Feb 26, 2018

infinity0 commented Mar 6, 2018

alexcrichton commented Mar 6, 2018

infinity0 commented Mar 7, 2018

alexcrichton commented Mar 7, 2018

bors commented Mar 7, 2018

bors commented Mar 7, 2018

bors commented Mar 7, 2018

		@@ -175,7 +175,11 @@ fn install_one(root: &Filesystem,

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

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

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

Conversation

infinity0 commented Feb 5, 2018

rust-highfive commented Feb 5, 2018

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinity0 commented Feb 9, 2018

infinity0 commented Feb 16, 2018

bors commented Feb 21, 2018

alexcrichton commented Feb 26, 2018

infinity0 commented Mar 6, 2018

alexcrichton commented Mar 6, 2018

infinity0 commented Mar 7, 2018

alexcrichton commented Mar 7, 2018

bors commented Mar 7, 2018

bors commented Mar 7, 2018

bors commented Mar 7, 2018