pksunkara / cargo-workspaces

A tool for managing cargo workspaces and their crates, inspired by lerna
MIT License
483 stars 46 forks source link

Add tests for `--dry-run` and `cargo ws plan` #157

Open popzxc opened 4 months ago

popzxc commented 4 months ago

I finally managed to run tests locally :)

This PR:

popzxc commented 3 months ago

@pksunkara I've experimented with cargo ws publish --dry-run a bit more, and it seems that there are still unsolved issues. For example, it doesn't seem to work with first-time-ever publishing (see sample test failure). Also in some of the workspaces I've checked, I still encountered issues with dependency resolution.

What would be the better way forward here? Add an extra flag to not perform cargo publish --dry-run (e.g. stop at build and basic checks)?

pksunkara commented 3 months ago

Also in some of the workspaces I've checked, I still encountered issues with dependency resolution.

Can you please describe them?

it doesn't seem to work with first-time-ever publishing (see sample test failure).

From what I read, the --no-verify should take care of it. Also, I tested this on my monorepo with non-published public crates and it was working.

popzxc commented 3 months ago

Can you please describe them?

Besides one that was mentioned (publishing for the first time), I've met the following error when trying to bump packages in workspace from 0.1.0-rc.2 to 0.2.0-rc.2:

    Updating crates.io index
error: failed to prepare local package for uploading

Caused by:
  failed to select a version for the requirement `<ws_dependency_name> = "=0.2.0-rc.2"`
  candidate versions found which didn't match: 0.1.0-rc.2, 0.1.0-rc.1
  location searched: crates.io index
  required by package `<ws_dependency_name> v0.2.0-rc.2 (<path>)`
  if you are looking for the prerelease package it needs to be specified explicitly
      <ws_dependency_name> = { version = "0.1.0-rc.2" }
  perhaps a crate was updated and forgotten to be re-vendored?

It's pretty weird and I'm not sure why it happens. My current hypothesis is that it's because these packages have build scripts, but I didn't investigate further so far.

pksunkara commented 3 months ago

publishing for first time

It seems to work. Try the following:

●  → cargo info random28
Error: Resouce at url 'https://crates.io/api/v1/crates/random28' could not be found
●  → cargo new random28
     Created binary (application) `random28` package
●  → cd random28
● /random28  (stable)  (empty) → cargo ws publish --dry-run --allow-dirty --publish-as-is
warn Dry run doesn't check that all dependencies have been published.
error: config value `http.cainfo` is not set
info checking random28
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s
warn check failed 'description' field should be set
warn check failed either 'license' or 'license-file' field should be set
    Updating crates.io index
warning: manifest has no description, license, license-file, documentation, homepage or repository.
See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
   Packaging random28 v0.1.0 (/home/pksunkara/Coding/termapps/random28)
    Packaged 4 files, 920.0B (712.0B compressed)
   Uploading random28 v0.1.0 (/home/pksunkara/Coding/termapps/random28)
warning: aborting upload due to dry run
info success ok
popzxc commented 3 months ago

This is probably becasue it's the only crate in the workspace (no ws deps). Try this:

$ cd cargo-workspaces/fixtures/normal
$ cargo ws publish --dry-run
warn Dry run doesn't check that all dependencies have been published. 
warn Dry run doesn't perform versioning. 
error: config value `http.cainfo` is not set
info checking dep1
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s
    Updating crates.io index
warning: manifest has no documentation, homepage or repository.
See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
   Packaging dep1 v0.1.0 (/<...>/cargo-workspaces/fixtures/normal/dep1)
    Packaged 4 files, 925.0B (715.0B compressed)
   Uploading dep1 v0.1.0 (/<...>/cargo-workspaces/fixtures/normal/dep1)
warning: aborting upload due to dry run
info checking dep2
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s
    Updating crates.io index
warning: manifest has no documentation, homepage or repository.
See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
   Packaging dep2 v0.1.0 (/<...>/cargo-workspaces/fixtures/normal/dep2)
    Packaged 4 files, 1.0KiB (756.0B compressed)
   Uploading dep2 v0.1.0 (/<...>/cargo-workspaces/fixtures/normal/dep2)
warning: aborting upload due to dry run
info checking top
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s
    Updating crates.io index
warning: manifest has no documentation, homepage or repository.
See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
   Packaging top v0.1.0 (/<...>/cargo-workspaces/fixtures/normal/top)
    Updating crates.io index
error: failed to prepare local package for uploading

Caused by:
  no matching package named `dep1` found
  location searched: registry `crates-io`
  required by package `top v0.1.0 (/<...>/cargo-workspaces/fixtures/normal/top)`
warn publish failed top v0.1.0
info success ok
pksunkara commented 3 months ago

I have a ws project with deps where some of them are not published yet, and it's working in there. But it's not working in normal fixture. And I was trying with cargo publish --dry-run --no-verify directly.

There are a few differences between both and I have been trying to pinpoint the issue, but unfortunately, I was not able to yet.

popzxc commented 3 months ago

Likewise. So, would it make sense to add an option to skip --dry-run step until we figure it out?

pksunkara commented 3 months ago

I will try to debug more on Sunday. I am currently thinking it's a bug in cargo itself. Will decide after that.

Skipping means, we have to skip the whole publish command.

popzxc commented 3 months ago

@pksunkara sorry for pinging, have you been able to find out the reason? I'm thinking on what to do with this PR. In theory, I can try to create a different fixture so that the test passes, but not sure if that's the correct approach.

pksunkara commented 3 months ago

I haven't had time to debug this, but let's resolve this week either way. Only one thing I would want to know is how does https://github.com/crate-ci/cargo-release dry run work on the same fixture? Does it give the same error as us?

popzxc commented 3 months ago

@pksunkara sorry, had a busy week.

Looks like yes, cargo-release fails with the same error. Here's (trimmed) output of cargo release publish for the fixtures/normal:

error: failed to prepare local package for uploading

Caused by:
  no matching package named `dep1` found
  location searched: registry `crates-io`

Moreover, looks like there is an open issue about that there.

It doesn't look like it's easily solvable, so IMHO it probably makes sense to allow skipping actual --dry-run step, and check if the package passes basic checks & can be built only. This is not as comprehensive, of course, but I think it's a nice compromise for end users.

pksunkara commented 3 months ago

Reading that issue, it looks like --no-verify already allows people to skip it. Can you please confirm?

If it doesn't, then yeah, we should allow that.

popzxc commented 3 months ago

I think the comments there refer to some internal behavior in cargo-release, because cargo publish --dry-run --no-verify skips the build but still checks dependencies, and thus fails:

cargo publish -p top --dry-run --no-verify
    Updating crates.io index
warning: manifest has no description, license, license-file, documentation, homepage or repository.
See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
   Packaging top v0.1.0 (/home/<..>/cargo-workspaces/fixtures/normal/top)
    Updating crates.io index
error: failed to prepare local package for uploading

Caused by:
  no matching package named `dep2` found
  location searched: registry `crates-io`
  required by package `top v0.1.0 (/home/<..>/cargo-workspaces/fixtures/normal/top)`
popzxc commented 2 months ago

@pksunkara Hey! Sorry, it's been a while, but I still want to drive this forward 😅

Could you please say which option sounds acceptable to you? I see a few options here:

  1. Add a new CLI flag, e.g. --skip-upload, which will be used together with --dry-run to not actually perform cargo publish --dry-run step.
  2. Split basic checks and dry-run into two separate CLI flags (though basic checks would still prevent publishing).
  3. Allow "customizing" --dry-run flag, e.g. it's possible to run cargo ws publish --dry-run=full (basic & publish), cargo ws publish --dry-run=basic (only basic), cargo ws publish --dry-run=publish (only publish), or simply cargo ws publish --dry-run (defaults to full.

I'm open to other suggestions as well. What would you prefer?