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 path dependencies without a version (RFC2906) #11133

Open
vladimir-lu opened this issue Sep 22, 2022 · 13 comments
Open

Allow publishing with path dependencies without a version (RFC2906) #11133

vladimir-lu opened this issue Sep 22, 2022 · 13 comments
Labels
A-manifest Area: Cargo.toml issues A-workspaces Area: workspaces C-enhancement Category: enhancement S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@vladimir-lu
Copy link

Problem

RFC #2906 specifies that you "No longer need both version and path to publish to Crates.io" here. Now that the tracking issue #8415 is in stable I feel like it's a missing feature aka a bug.

The problem is the one described in the RFC - a multi-member workspace should be able to specify mod-a = { path = "path-to-mod-a" } without specifying the version when publishing to crates.io.

Steps

  1. Create a multi-member workspace
  2. Add a dependency from one crate to another using only path = 'path'
  3. Get the error: error: all dependencies must have a version specified when publishing.

Possible Solution(s)

No response

Notes

My apologies if this was already covered somewhere else but a search and reading through the comments on the RFC didn't yield anything about why this is not implemented. Or perhaps I just haven't found the way to do this yet and the docs need an update to clarify.

Version

cargo 1.64.0 (387270bc7 2022-09-16)
release: 1.64.0
commit-hash: 387270bc7f446d17869c7f208207c73231d6a252
commit-date: 2022-09-16
host: x86_64-unknown-linux-gnu
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.83.1-DEV (sys:0.4.55+curl-7.83.1 vendored ssl:OpenSSL/1.1.1q)
os: Fedora 36 (ThirtySix) [64-bit]
@vladimir-lu vladimir-lu added the C-bug Category: bug label Sep 22, 2022
@epage
Copy link
Contributor

epage commented Sep 22, 2022

In #8415, we deviated from the RFC, including

Not inserting a version into path dependencies when publishing, see #8415 (comment)

@epage epage added A-manifest Area: Cargo.toml issues A-workspaces Area: workspaces labels Sep 22, 2022
@vladimir-lu
Copy link
Author

@epage Thank you for pointing me to the discussion. Do you know if something was decided about this in the future (perhaps a workspace flag like for other workspace-inheritance related usages right now)? Otherwise I guess this can be closed.

@epage
Copy link
Contributor

epage commented Sep 22, 2022

If it were to move forward, it will need to be re-examined from scratch. It didn't get the attention it needed in the initial RFC and has breaking changes and bad workflows that would need to be addressed. While its a neat idea, I don't think it would end up being worth the effort in practice.

@vladimir-lu
Copy link
Author

As a user, the ability to keep the version consistent across the entire repo (here represented by a cargo workspace) is very useful, even if it means republishing new versions for crates where nothing changed, because of the simplicity of dealing with a single version rather than multiple. I can also see the argument why semver might imply that you should probably publish the most minimal version increment possible (what was being argued in the RFC comments), though for monorepo-managed applications that are also published to a registry there doesn't seem much practical benefit vs. the library case.

Right now, the workaround is to use cargo-edit (and I guess killercup/cargo-edit#752 already tracks the change there) which is good enough in practice for me.

@epage epage added C-enhancement Category: enhancement and removed C-bug Category: bug labels Sep 27, 2022
@epage epage changed the title Path dependencies are not allowed to be published when using workspace version inheritance (RFC2906) Allow publishing with path dependencies without a version (RFC2906) Sep 27, 2022
fundon added a commit to viz-rs/viz that referenced this issue Dec 30, 2022
fundon added a commit to viz-rs/viz that referenced this issue Feb 12, 2023
* main: (37 commits)
  chore(core): csrf: use base64 0.21 api
  chore(deps): update
  chore(examples): limits (#69)
  chore(core): deps: cookie-0.17
  fix: limits should be sorted
  chore(deps): core: form-data-v0.5.0-rc.1
  chore(funding): update
  chore(funding): update
  doc: fix discord link
  doc: update viz link
  chore: docs
  chore(core): body: poll ?
  fix: cargo
  fix(cargo): rust-lang/cargo#11133
  fix(core): clippy
  chore(core): clippy
  fix(core): clippy
  chore(core): clippy
  doc: add langs links
  chore(deps): hyper v1.0.0-rc.2
  ...
aschaeffer added a commit to reactive-graph/reactive-graph that referenced this issue Apr 30, 2023
@epage epage added the S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. label Oct 27, 2023
@epage
Copy link
Contributor

epage commented Oct 30, 2023

Of note, this request is the opposite of #6126

@SamB
Copy link

SamB commented Mar 9, 2024

If it were to move forward, it will need to be re-examined from scratch. It didn't get the attention it needed in the initial RFC and has breaking changes and bad workflows that would need to be addressed. While its a neat idea, I don't think it would end up being worth the effort in practice.

Um, isn't the idea to publish all of the packages that inherit their version from the workspace in one go? I guess this would need protocol changes, but does it actually break any code or workflows?

I can't see any way to interpret setting version.workspace = true for several packages that doesn't imply "please version these in lockstep".

In any case, it looks like the RFC needs more notes about stuff that got changed during implementation...

github-merge-queue bot pushed a commit to NomicFoundation/slang that referenced this issue Jun 12, 2024
Currently our publishing pipeline is failing to publish workspace crates
that have internal dependencies:

https://github.com/NomicFoundation/slang/actions/runs/9468852124/job/26088399371

```log
error: all dependencies must have a version specified when publishing.
  dependency `metaslang_cst` does not specify a version
```

This is a known bug/limitation of `cargo publish`:
rust-lang/cargo#11133

To get around this, I'm explicitly adding versions to all internal
dependencies, and switching our versioning workflow to use `cargo
set-version` to update all of them when creating the changesets PR.
sdankel added a commit to FuelLabs/sway that referenced this issue Sep 25, 2024
## Description

This should
[unblock](https://github.com/FuelLabs/sway/actions/runs/11035362451/job/30652169645#step:4:22)
releases. [Cargo doesn't use the workspace version for path
dependencies](rust-lang/cargo#11133), so we
still have to specify the versions. This is also how it's done in
[fuel-core](https://github.com/FuelLabs/fuel-core/blob/master/Cargo.toml).

This is actually nice because it means we could in the future bump the
versions of the dependencies separately, all from within the one file.

## Checklist

- [ ] I have linked to any relevant issues.
- [ ] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [ ] If my change requires substantial documentation changes, I have
[requested support from the DevRel
team](https://github.com/FuelLabs/devrel-requests/issues/new/choose)
- [ ] I have added tests that prove my fix is effective or that my
feature works.
- [ ] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [ ] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [ ] I have requested a review from the relevant team or maintainers.
@heaths
Copy link

heaths commented Nov 12, 2024

We just ran into this as well. We have a soon-to-be-large monorepo where we want to manage all dependencies - including intra-package dependencies - centrally in a workspace. The workspace defines all the third-party [dependencies] and, for intra-package dependencies, defines them with a path. Based on some discussion in #8415, we were assuming crates' versions would be "harvested" at package/publish time to whatever is currently defined in the dependencies' Cargo.toml manifests.

@epage looking at the comment you linked earlier, I appreciate the edge cases. Would it be too naive to 1) err on cycles within the same dependencies section, but 2) allow cycles across different dependencies sections e.g. between dependencies and dev-dependencies? Of course, those would have to be resolved for each package since, it seems, pure workspaces can only declare dependencies in dependencies, from which any dependencies section can inherit. At least, that seems to be the case in my earlier exploration.

/cc @hallipr

@epage
Copy link
Contributor

epage commented Nov 12, 2024

Um, isn't the idea to publish all of the packages that inherit their version from the workspace in one go? I guess this would need protocol changes, but does it actually break any code or workflows?

Implicit version in dependencies is a separate feature from package.version.workspace = true and requiring the latter would severely limit who could use it.

EDIT: Looks like this was specifically in reply to #8415 (comment). In that case, no it isn't. I use package.version for clap but don't re-release each piece of clap intentionally.

In any case, it looks like the RFC needs more notes about stuff that got changed during implementation...

RFCs are snapshots in time for the decision that was made and are not living specifications for behavior. We called out the deviations from the RFC in #8415.

@epage
Copy link
Contributor

epage commented Nov 12, 2024

@heaths in the example given, how would we deal with a clap_derive that depends on clap? The cycles are between sections. If I try to publish clap_derive, it will do a test job and find that no version exists for the clap dependency.

As for any heuristics for this, something important to keep in mind is that we are a bit uncomfortable with the current level of complexity for parsing of manifests, particularly workspace inheritance. Recently, we backed out an Edition 2024 change because it hit unexpected aspects of manifest parsing that were going to take the complexity too far and we're going to have to re-evaluate things and write up a new RFC.

@heaths
Copy link

heaths commented Nov 12, 2024

If I try to publish clap_derive, it will do a test job and find that no version exists for the clap dependency.

@epage could you clarify "do a test job"? Crates.io? Clap's build process? That would help to understand. But since clap_derive is taking a dev-dependency on clap, would it make sense - especially since I'm assuming it's also specifying a path - to just keep or even infer the dependency as a wildcard dependency? In practice, seems it shouldn't matter.

@epage
Copy link
Contributor

epage commented Nov 12, 2024

could you clarify "do a test job"? Crates.io? Clap's build process?

The verify build step.

But since clap_derive is taking a dev-dependency on clap, would it make sense - especially since I'm assuming it's also specifying a path - to just keep or even infer the dependency as a wildcard dependency? In practice, seems it shouldn't matter.

I feel like there are important details elided here. Who is doing this, how does it know to do this, and how will crates.io deal with it when it bans wildcard dependencies?

@heaths
Copy link

heaths commented Nov 12, 2024

Who is doing this, how does it know to do this

Couldn't the crate authors do this for dev-dependencies i.e., author a wildcard version if they need to declare a cyclic dependency otherwise? Though...

how will crates.io deal with it when it bans wildcard dependencies?

I didn't realize this, though it does make some sense (certainly for dependencies). I see at some point something e.g., cargo, is going to have to make some changes to the manifest with fixed versions. But does crates.io really need to be concerned with anything other than normal dependencies (including target-specific .dependencies)? Hmm, well, I suppose it does need to care about build-dependencies too e.g., for -sys crates.

@epage
Copy link
Contributor

epage commented Nov 13, 2024

Something else i forgot to call out is that requiring maintainers make changes for this would require an Edition, if that could even handle it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-manifest Area: Cargo.toml issues A-workspaces Area: workspaces C-enhancement Category: enhancement S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
Development

No branches or pull requests

4 participants