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

--no-dev-deps flag edits real manifest #15

Closed
taiki-e opened this issue Nov 4, 2019 · 8 comments
Closed

--no-dev-deps flag edits real manifest #15

taiki-e opened this issue Nov 4, 2019 · 8 comments
Labels
A-dev-deps Area: dev-dependencies (--no-dev-deps, --remove-dev-deps, etc.)

Comments

@taiki-e
Copy link
Owner

taiki-e commented Nov 4, 2019

Using --no-dev-deps flag is recommended to avoid rust-lang/cargo#4866 if resolver = "2" doesn't set.

However, currently, using --no-dev-deps flag removes dev deps from real manifest while cargo-hack is running and restores it when finished.
If possible, this should be done without editing the manifest.

Workaround

Using unstable -Z features=dev_dep (rust-lang/cargo#7916) or -Z avoid-dev-deps (rust-lang/cargo#5133) instead of --no-dev-deps.

Note:
-Z avoid-dev-deps works well with cargo check/build/run/install, but not with cargo publish.
On the other hand, --no-dev-deps doesn't work well with cargo install.

@taiki-e taiki-e added C-bug Category: related to a bug. C-enhancement Category: A new feature or an improvement for an existing one labels Nov 4, 2019
@taiki-e taiki-e added this to the v0.3 milestone Nov 4, 2019
@taiki-e
Copy link
Owner Author

taiki-e commented Nov 4, 2019

Perhaps it needs to ban test/bench subcommands and --tests/--all-targets flags when --no-dev-deps is used. (done in #16)

Then create an empty crate for testing in the target directory like trybuild is doing.

@taiki-e taiki-e self-assigned this Nov 4, 2019
@taiki-e
Copy link
Owner Author

taiki-e commented Nov 4, 2019

Then create an empty crate for testing in the target directory like trybuild is doing.

Hmm, if doing this, it will not work well except for check subcommand (especially publish subcommand).

@taiki-e
Copy link
Owner Author

taiki-e commented Nov 4, 2019

So, for now, I think it should handle the check subcommand specially. (And probably warn when used with other subcommands.)

bors bot added a commit that referenced this issue Nov 6, 2019
16: Ban --no-dev-deps with builds that require dev-dependencies r=taiki-e a=taiki-e

cc #15 

Co-authored-by: Taiki Endo <[email protected]>
@taiki-e taiki-e modified the milestones: v0.3, v0.4 Nov 12, 2019
@taiki-e taiki-e mentioned this issue Nov 20, 2019
bors bot added a commit that referenced this issue Nov 20, 2019
20: Handle ctrl-c r=taiki-e a=taiki-e

cc #15 

Co-authored-by: Taiki Endo <[email protected]>
@taiki-e
Copy link
Owner Author

taiki-e commented Nov 22, 2019

Now that cargo-hack can handle ctrl-c properly, I think that some burden caused by this issue has been reduced.

@taiki-e
Copy link
Owner Author

taiki-e commented Feb 1, 2020

Workaround for this is using unstable -Z avoid-dev-deps instead of --no-dev-deps.

-Z avoid-dev-deps (tracking-issue: rust-lang/cargo#5133) is an unstable feature of cargo that has almost the same effect as cargo-hack's --no-dev-deps. However, unlike --no-dev-deps, the manifest file is not edited.

FWIW:
-Z avoid-dev-deps works well with cargo check/build/run/install, but not with cargo publish.
On the other hand, --no-dev-deps doesn't work well with cargo install.

@taiki-e taiki-e removed this from the v0.4 milestone Apr 23, 2020
@taiki-e taiki-e removed the C-bug Category: related to a bug. label May 17, 2020
@taiki-e taiki-e changed the title Do not edit real manifest in --no-dev-deps --no-dev-deps flag edits real manifest Jul 11, 2020
@taiki-e
Copy link
Owner Author

taiki-e commented Jul 11, 2020

We can probably consider using -Z avoid-dev-deps or -Z features=dev_dep for some subcommands in nightly if they can guarantee some equivalent behavior.
But I'm not sure if that is the desired behavior.

@taiki-e taiki-e added C-tracking-issue C-enhancement Category: A new feature or an improvement for an existing one and removed C-enhancement Category: A new feature or an improvement for an existing one labels Oct 2, 2020
@taiki-e taiki-e removed C-enhancement Category: A new feature or an improvement for an existing one C-tracking-issue labels Oct 19, 2020
@taiki-e taiki-e added the A-dev-deps Area: dev-dependencies (--no-dev-deps, --remove-dev-deps, etc.) label Nov 11, 2020
@jhpratt
Copy link
Contributor

jhpratt commented Aug 24, 2021

With resolver = "2" stabilized (and becoming default in the 2021 edition), I don't see this as a particularly "must-fix" kind of issue.

@taiki-e taiki-e closed this as completed Aug 24, 2021
@taiki-e
Copy link
Owner Author

taiki-e commented Mar 2, 2022

and becoming default in the 2021 edition

BTW, at least when the workspace root is a virtual manifest, resolver = "2" does not seem to properly apply in the workspace, even if all workspace members are 2021 edition.

Therefore, in practice, it seems preferable to always specify resolver = "2" (considering the possibility that the project structure may change in the future).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dev-deps Area: dev-dependencies (--no-dev-deps, --remove-dev-deps, etc.)
Projects
None yet
Development

No branches or pull requests

2 participants