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

Allow publishing with dev-dependencies without a version. #7333

Open
wants to merge 2 commits into
base: master
from

Conversation

@ehuss
Copy link
Contributor

commented Sep 5, 2019

This change allows dev-dependencies without a version key to be published. If a dev-dependency is missing the version, it will be stripped from the packaged manifest.

@rust-highfive

This comment has been minimized.

Copy link

commented Sep 5, 2019

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

@rfcbot fcp merge

I've personally always wanted this! Curious what others think though

@rfcbot

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2019

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@Eh2406

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

cc Previous discussion at rust-lang/crates-io-cargo-teams#46 & rust-lang/crates.io#1789.

I thought the consensus was that we wanted the .crate file to still work and have crates.io except the dev-dep?

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

One potential concern is that this means that downloading a crate file, like e.g. Crater does, and running cargo test is presumably much more likely to not work, which isn't great.

However, maybe we can get around this by requiring that the dev-deps are still present in the .crate file (obviously, this can be bypassed with --no-verify, but that's not particularly interesting).

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

@ehuss can correct me, but while it doesn't look like there's an explicit test the Cargo.toml inside the *.crate file still has [dev-dependencies] so long as they have a version listed. It's just the blob we send to crates.io that has them filtered out. They won't show up in the crates.io UI any more.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

Ah, then that seems fine to me. Maybe good to add a test if it's relatively simple though.

@Eh2406

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

+1 on a test! Also since we repeatedly took time at the crates.io meetings, should we ask for there checkmarks?

@ehuss

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

but while it doesn't look like there's an explicit test the Cargo.toml inside the *.crate file still has [dev-dependencies] so long as they have a version listed. It's just the blob we send to crates.io that has them filtered out. They won't show up in the crates.io UI any more.

Sorry for the confusion. If a dev-dependency does not specify version, it is filtered from both crates.io and the generated Cargo.toml file. If it does include version, there is no change in behavior.

I can actually remove the generated Cargo.toml stripping. I was initially thinking it might cause problems if the registry Summary was out of sync with Cargo.toml, but I did some tests and it seems fine. Essentially just remove all the changes to toml/mod.rs in this PR. Do people think it would be a good idea to make that change? The only purpose would be to support these path-only deps for a build-and-test run on crater. I think if we do that, we should also have some checks that the dependency is actually included in the package, and deal with .. (I'm not sure how complex that will be). It seems like a bit of effort for a marginal benefit, though.

I can work on better tests. I'm thinking of writing something that will actually commit the publish to the local registry.

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

I'd like to have some verification that the referenced dev-dependency not only has a path inside the crate, but that the referenced path actually will get included in the crate and contains a Cargo.toml file.

(That was also the consensus from the crates.io team.)

@Eh2406

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

@rfcbot concern continue discussion with crates.io

Previous discussion at rust-lang/crates-io-cargo-teams#46 & rust-lang/crates.io#1789 let's continue the discussion in one of those threads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.