crate-ci / cargo-release

Cargo subcommand `release`: everything about releasing a rust crate.
Apache License 2.0
1.34k stars 113 forks source link

cargo-release doesn't prevent publishing a package when it has unpublished registry dependencies #624

Open kornelski opened 1 year ago

kornelski commented 1 year ago

If I run cargo release in the root of: https://github.com/kornelski/cavif-rs commit b2f707ce08bf758048cb967998df92d8dd3d2c7c

https://github.com/kornelski/cavif-rs/tree/b2f707ce08bf758048cb967998df92d8dd3d2c7c/

it chooses not to publish workspace's leaf dependency first (ravif), and then the root crate fails validation (cavif), because its dependency isn't published yet.

$ cargo release
warning: disabled by user, skipping ravif which has files changed since v1.4.0: [
    "./cavif/ravif/Cargo.toml",
    "./cavif/ravif/src/av1encoder.rs",
    "./cavif/ravif/src/dirtyalpha.rs",
    "./cavif/ravif/src/error.rs",
    "./cavif/ravif/src/lib.rs",
]
warning: disabled by user, skipping ravif v0.9.3 despite being unpublished
  Publishing cavif
warning: profiles for the non root package will be ignored, specify profiles at the workspace root:
package:   ./cavif/ravif/Cargo.toml
workspace: ./cavif/Cargo.toml
    Updating crates.io index
   Packaging cavif v1.4.1 (./cavif)
error: failed to prepare local package for uploading

Caused by:
  failed to select a version for the requirement `ravif = "^0.9.2"`
  candidate versions found which didn't match: 0.9.1, 0.9.0, 0.8.9, ...
  location searched: crates.io index
  required by package `cavif v1.4.1 (./cavif)`

I don't see why cargo-release thinks the dependency is "disabled by user".

Using cargo-release 0.24.0 and rustc 1.65

epage commented 1 year ago

cargo release mimics most cargo command when selecting packages. Running in a virtual workspace will select everything but a workspaced with a package will select just that package. You need to either say -p ravif, --workspace, or --unpublished.

kornelski commented 1 year ago

That's disappointing. I'd expect it to be smarter than cargo publish and figure out it needs to publish dependencies first.

epage commented 1 year ago

Actually, that is why I hadn't closed this yet.

We should check all registry dependencies of the item we are publishing and ensure they are published as well.

Unsure whether this should be a warning, error, or that we implicitly publish them. I lean towards error. A warning is good if there could be a valid reason for someone not doing it but this will always fail. If we implicitly publish, the user might not have prepared that package for publishing and we could be doing something unexpected. An error gives the user the ability to choose whether it is right or not. We still have --unpublished to make it easier to publish them

epage commented 1 year ago

One thing I'll add is that we have the following warning

warning: disabled by user, skipping ravif v0.9.3 despite being unpublished

This is generic and doesn't deal with dependencies. While useful to help users identify crates they might have meant to also include, relying solely on this warning to resolve this issue means the user has to mentally track the dependency tree.