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 crates to be published with cyclic dev-dependencies #4242

Open
Marwes opened this issue Jul 1, 2017 · 43 comments
Open

Allow crates to be published with cyclic dev-dependencies #4242

Marwes opened this issue Jul 1, 2017 · 43 comments
Labels
A-dev-dependencies Area: [dev-dependencies] Command-publish S-triage Status: This issue is waiting on initial triage.

Comments

@Marwes
Copy link

Marwes commented Jul 1, 2017

In gluon I have the project split up into multiple crates with the intent that users can include only the crates they need if they desire a smaller binary footprint. For convenience I provide a main gluon crate which provides an easy to use interface when one uses all the crates.

While this split works fine for mostly everything I want to use the easy interface when writing documentation tests so I added the gluon crate as a dev-dependency in gluon_vm and this compiles without issue since a little bit back. It does however break when trying to publish gluon_vm as the gluon dev-dependency has not been published yet!

Currently I managed to work around it by specifying a version range but it would be nice if there was a way to publish a group of crates as a whole or to ignore the version check if a dev-dependency does not exist on crates.io (yet).

gluon = { path = "..", version = "<0.6.0, >=0.4.2" } # GLUON
@alexcrichton
Copy link
Member

Yeah in general I think we should basically ignore dev-dependencies when we publish. We shouldn't attempt to resolve them, verify they're already published, etc.

@BusyJay
Copy link
Contributor

BusyJay commented Jul 25, 2017

Maybe dev-dependencies don't have to be shown in the crate's page too.

@matthewkmayer
Copy link

I bumped into this issue as well. From what I can tell the workaround above gets us by this for now. 👍

@cramertj
Copy link
Member

cramertj commented Mar 5, 2018

+1 to this-- it now affects the futures crate.

malikolivier added a commit to malikolivier/imgui-rs that referenced this issue Aug 11, 2018
While preparing for publication, I realized I cannot publish crates
with cyclic dev-dependencies.
That's an interesting issue, and a work-around is currently used:
rust-lang/cargo#4242

Another issue: it seems "imgui-glium-renderer" requires the "glutin"
feature of "glium". However it was not enabled. So I enabled it.
malikolivier added a commit to malikolivier/imgui-rs that referenced this issue Aug 11, 2018
While preparing for publication, I realized I cannot publish crates
with cyclic dev-dependencies.
That's an interesting issue, and a work-around is currently used:
rust-lang/cargo#4242

Another issue: it seems "imgui-glium-renderer" requires the "glutin"
feature of "glium". However it was not enabled. So I enabled it.
malikolivier added a commit to malikolivier/imgui-rs that referenced this issue Aug 12, 2018
While preparing for publication, I realized I cannot publish crates
with cyclic dev-dependencies.
That's an interesting issue, and a work-around is currently used:
rust-lang/cargo#4242

Another issue: it seems "imgui-glium-renderer" requires the "glutin"
feature of "glium". However it was not enabled. So I enabled it.
malikolivier added a commit to malikolivier/imgui-rs that referenced this issue Aug 13, 2018
While preparing for publication, I realized I cannot publish crates
with cyclic dev-dependencies.
That's an interesting issue, and a work-around is currently used:
rust-lang/cargo#4242
malikolivier added a commit to malikolivier/imgui-rs that referenced this issue Aug 13, 2018
While preparing for publication, I realized I cannot publish crates
with cyclic dev-dependencies.
That's an interesting issue, and a work-around is currently used:
rust-lang/cargo#4242
malikolivier added a commit to malikolivier/imgui-rs that referenced this issue Aug 13, 2018
While preparing for publication, I realized I cannot publish crates
with cyclic dev-dependencies.
That's an interesting issue, and a work-around is currently used:
rust-lang/cargo#4242
malikolivier added a commit to malikolivier/imgui-rs that referenced this issue Aug 14, 2018
While preparing for publication, I realized I cannot publish crates
with cyclic dev-dependencies.
That's an interesting issue, and a work-around is currently used:
rust-lang/cargo#4242
@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 15, 2018

This affects a lot of crates, what do we need to do to fix this? Isn't there a check somewhere that checks for the dependencies before publishing, where we can just filter dev-dependencies away and call it a day? [0]

I mean, this would even affect core and std, where std could depend on core to build, but core would depend on std for tests. The check is spurious anyways, because this sort of dependencies are not circular: build core -> build std -> build core tests is ok. Tests and "dev-only" artifacts are their own crates (core+tests is a different crate than core, and so are every test/foo.rs in core, and core/examples, etc.).

I'll admit that the core/std analogy is imperfect because these crates are too magical, but when something wouldn't be correct for the std library, chances are it won't be correct for users either.


FWIW I just tried to prepare coresimd/stdsimd for a quick crates.io release, and ran into this, where coresimd builds standalone and stdsimd depends on coresimd, but coresimd depends on stdsimd for testing.

The only solution I came up with is the same that gluon uses which is insane: publish coresimd prunning dev-dependencies, publish stdsimd, re-publish coresimd with a minor version update to use the newly publish stdsimd as dev-dependencies.

Which is a lot of effort, for.. what exactly? Is it possible for users to execute cargo test in their whole dependency graph?


[0] cc @matklad, are you willing to mentor this?

It might be enough to just skip dev dependencies here:

for dep in pkg.dependencies().iter() {

and also in packaging: https://github.com/rust-lang/cargo/blob/0e7a46e3276f56822b6ef48e341c522be27db17e/src/cargo/ops/cargo_package.rs

@matklad
Copy link
Member

matklad commented Aug 15, 2018

The only solution I came up with is the same that gluon uses which is insane: publish coresimd prunning dev-dependencies, publish stdsimd, re-publish coresimd with a minor version update to use the newly publish stdsimd as dev-dependencies.

Hm, I think a simpler work-around is possible? Just comment-out dev-dependencies when publishing. This is what we do in Cargo, which has a path = dev-dep.

For mentoring instructions, I believe you've basically nailed it :) The only thing I have to add is that the test should go to this file, and it probably makes sense to start with a test and work through all errors.

For the tests, I think we should cover at least two scenarios:

  • publishing a path dev-dep should not fail (slight modification of this test)
  • publishing with a "cycle" via a dev-dep should not fail (the original motivation for the issue).

@alexcrichton
Copy link
Member

Hm I'm actually not sure that the fix would go in that location nor that we could easily write a test for this. AFAIK the check for "does this dependency exist" is done on crates.io, not locally in Cargo. In that sense we'd need to test with crates.io, not with just local Cargo test suite business.

I also think the "fix" for this is to basically stop informing crates.io about dev-dependencies. Although we did that initially I'm not really sure why we need to do that, as they're never used and are otherwise just bloating the index. The fix for that would go around here by filtering out dev-dependencies. Cargo would then also need to be updated to not verify that dev-dependencies have versions (allowing path dependencies without a version).

This would be a relatively large change though (and somewhat of a policy shift) so it would likely need some more scrutiny and discussion before happening.

@matklad
Copy link
Member

matklad commented Sep 8, 2018

I also think the "fix" for this is to basically stop informing crates.io about dev-dependencies.

Hm, probably a crazy idea, but, given that we already modify Cargo.toml on publishing to rewrite path dependencies (source), could we, at the same step, just drop dev-deps altogether as well?

@alexcrichton
Copy link
Member

Yes I think such a fix would work, but it alone wouldn't be 100% sufficient because crates.io doesn't actually parse the manifest for dependency information (it only uses what was sent in JSON)

malikolivier added a commit to malikolivier/imgui-rs that referenced this issue Oct 9, 2018
While preparing for publication, I realized I cannot publish crates
with cyclic dev-dependencies.
That's an interesting issue, and a work-around is currently used:
rust-lang/cargo#4242
@jonhoo
Copy link
Contributor

jonhoo commented Nov 23, 2018

I guess the proposed change would also now allow git dependencies in dev-dependencies?

@jayvdb
Copy link

jayvdb commented May 9, 2023

futures/ci/remove-dev-dependencies uses toml_edit to remove dev-dependencies from a workspace in-place. It's used during CI for testing rather than publishing though, I'm not sure if you'd want to change anything about it for publish, but it's a pretty trivial tool.

It was removed in rust-lang/futures-rs@73ade1e1

It was replaced with https://github.com/taiki-e/cargo-hack#--no-dev-deps , which works for publishing.

@taiki-e
Copy link
Member

taiki-e commented May 9, 2023

Eventually, futures was able to avoid the need to edit the manifest before publishing by removing version from path dev-dependencies: rust-lang/futures-rs#2305

@epage
Copy link
Contributor

epage commented Oct 14, 2023

PR #7333 added auto-stripping of dev-dependencies that do not have a version specified. This was actually added for when a dev-dependency exists but isn't published. See rust-lang/crates-io-cargo-teams#46 & rust-lang/crates.io#1789. cargo add takes advantage of this by not adding the version field for path dev-dependencies. That gets us through cargo package verification and whatever crates.io does.

However, people using this, especially with cargo add making it the default, decreases the coverage we get from crater. The question is if there is anything more we can and should do.

One idea is to completely ignore dev-dependencies at each stage of the publish process. At minimum, we need to ensure the Cargo.lock is appropriate for uploading. Maybe we could skip that in some cases but now we need to find the right rules (or add configuration) to control this. I believe we already exclude transitive dev-dependencies when generating your Cargo.lock so I assume that should be fine if they don't exist. No idea if we can do similar for cargo install. So maybe something can be done but there are a lot of ifs and potential caveats to it. The question is if the pay off is enough for this at this point.

@epage
Copy link
Contributor

epage commented Apr 2, 2024

btw rust-lang/rfcs#3452 could allow nesting dev dependencies as another way of breaking cycles.

alessandrod added a commit to aya-rs/aya that referenced this issue Apr 6, 2024
This wasn't meant to be committed, cargo-smart-release. Commenting is
needed to fix the cyclic dep aya-ebpf-macros -> aya-ebpf ->
aya-ebpf-macros. See
rust-lang/cargo#4242 (comment)
@epage
Copy link
Contributor

epage commented Apr 24, 2024

Most of the time I see this discussed, it is in the context where a path dependency exists to a package that might not be published yet.

There are cases, like async-stream and tokio-test (see tokio-rs/async-stream#102) where path dependencies are not involved because its cross-repo. This simplifies the problem because the cyclic dev-dependencies have to be published for any of this to work. As mentioned earlier in this thread and again in https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Is.20dev-dependency.20information.20from.20the.20index.20used.3F, dev-dependencies don't matter for the index, so we could strip them from the Summary we upload. We could even keep them in the Cargo.toml for now to not interfere with crater.

madsmtm added a commit to madsmtm/objc2 that referenced this issue May 4, 2024
Dunqing pushed a commit to oxc-project/oxc that referenced this issue Jul 23, 2024
DonIsaac pushed a commit to oxc-project/oxc that referenced this issue Jul 23, 2024
DonIsaac pushed a commit to oxc-project/oxc that referenced this issue Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dev-dependencies Area: [dev-dependencies] Command-publish S-triage Status: This issue is waiting on initial triage.
Projects
None yet
Development

No branches or pull requests