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

popzxc · 2024-07-17T06:27:28Z

I finally managed to run tests locally :)

This PR:

Adds instructions on how to run tests locally.
~~Removes cargo publish --dry-run step from cargo ws publish --dry-run (see this comment).~~ Upd: reverted.
Adds tests for cargo ws publish --dry-run.
Adds tests for cargo ws plan

popzxc

A few comments to assist with the review.

popzxc · 2024-07-17T06:28:32Z

README.md

+The recommended way to run tests for the crate is as follows:
+
+```sh
+cargo test --manifest-path cargo-workspaces/Cargo.toml -j1


I'm not fully sure if that's an appropriate fix, but it seems to work consistently.
Without it, tests were always failing. Probably this issue may be related.

popzxc · 2024-07-17T06:30:23Z

README.md

+If you observe unexpected differences in snapshots, you may want to override your compiler to
+the same version as used in [CI](.github/workflows/ci.yml), e.g.:


CI is fixed on 1.70, and newer cargo doesn't emit # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html line.
Also, it looks like cargo install --path cargo-workspaces doesn't work with 1.70, so when I worked on it, I had to manually override compiler for tests and remove an override for installation.
I guess MSRV and snapshots should be updated, but I thought it'd be an unnecessary scope extension for this PR.

I believe that this comment is useful as a general observation regardless of that.

popzxc · 2024-07-17T06:30:58Z

cargo-workspaces/src/publish.rs

@@ -46,6 +46,7 @@ pub struct Publish {

 impl Publish {
    pub fn run(mut self, metadata: Metadata) -> Result {
+        let mut command_success = true;


Since we no longer bail on errrors, I thought that it's weird to print success ok if we've met problems.

README.md

pksunkara · 2024-07-17T09:56:46Z

cargo-workspaces/src/publish.rs

@@ -100,7 +101,10 @@ impl Publish {
                    warn!("build failed", "");
                }

-                basic_checks(pkg)?;
+                command_success = command_success && basic_checks(pkg)?;
+                // We cannot use `cargo publish --dry-run` directly, because it checks if all the dependencies


I kept the basic checks, but I was wondering if we still need them if --dry-run warns about all of the issues that we check here?

No, --dry-run doesn't warn about them -- they are only checked during uploading.
Probably it makes sense to add this functionality to cargo directly, I'll create an issue there.

Yeah, that's probably the way we want to go in the future.

cargo-workspaces/src/publish.rs

cargo-workspaces/src/utils/basic_checks.rs

popzxc · 2024-07-17T10:36:20Z

@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 · 2024-07-17T10:40:40Z

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.

pksunkara · 2024-07-17T10:41:52Z

CONTRIBUTING.md

+The recommended way to run tests for the crate is as follows:
+
+```sh
+cargo test --manifest-path cargo-workspaces/Cargo.toml -j1


You don't need the j1 if you add #[serial] to all the tests.

pksunkara · 2024-07-17T10:42:49Z

cargo-workspaces/tests/publish.rs

+mod utils;
+use insta::assert_snapshot;
+
+#[test]


I see some test failures caused by parallel execution. #[serial] should be added to all these tests.

popzxc · 2024-07-17T11:31:17Z

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 · 2024-07-18T01:26:56Z

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 · 2024-07-18T03:15:02Z

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 · 2024-07-19T11:47:32Z

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 · 2024-07-19T13:55:50Z

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

pksunkara · 2024-07-19T14:32:07Z

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 · 2024-07-24T05:36:01Z

@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 · 2024-08-04T12:54:35Z

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 · 2024-08-09T05:21:22Z

@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 · 2024-08-09T10:37:29Z

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 · 2024-08-09T13:52:49Z

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 · 2024-09-02T06:49:03Z

@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:

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.
Split basic checks and dry-run into two separate CLI flags (though basic checks would still prevent publishing).
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?

popzxc commented Jul 17, 2024

View reviewed changes

popzxc force-pushed the dry-run-fix-plus-tests branch 2 times, most recently from c103dcd to 5a2e280 Compare July 17, 2024 06:54

popzxc mentioned this pull request Jul 17, 2024

feat: Implement print plan and basic checks for publish command #154

Merged

pksunkara reviewed Jul 17, 2024

View reviewed changes

popzxc force-pushed the dry-run-fix-plus-tests branch from 5a2e280 to b20f81e Compare July 17, 2024 10:31

popzxc added 4 commits July 17, 2024 14:32

Fix ws publish --dry-run

a1fc609

Add tests for cargo ws plan

6b45de9

Handle ascii control in output normalization

8592910

Create CONTRIBUTING.md

50d83ea

popzxc force-pushed the dry-run-fix-plus-tests branch from b20f81e to 50d83ea Compare July 17, 2024 10:32

popzxc changed the title ~~Fix cargo ws publish --dry-run, add tests for --dry-run and cargo ws plan~~ Add tests for --dry-run and cargo ws plan Jul 17, 2024

pksunkara reviewed Jul 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

popzxc commented Jul 17, 2024 •

edited

Loading

popzxc left a comment

popzxc Jul 17, 2024

popzxc Jul 17, 2024

popzxc Jul 17, 2024

pksunkara Jul 17, 2024

popzxc Jul 17, 2024

pksunkara Jul 17, 2024

popzxc commented Jul 17, 2024

pksunkara commented Jul 17, 2024

pksunkara Jul 17, 2024

pksunkara Jul 17, 2024

popzxc commented Jul 17, 2024

pksunkara commented Jul 18, 2024

popzxc commented Jul 18, 2024

pksunkara commented Jul 19, 2024

popzxc commented Jul 19, 2024

pksunkara commented Jul 19, 2024

popzxc commented Jul 24, 2024

pksunkara commented Aug 4, 2024

popzxc commented Aug 9, 2024 •

edited

Loading

pksunkara commented Aug 9, 2024

popzxc commented Aug 9, 2024

popzxc commented Sep 2, 2024

		If you observe unexpected differences in snapshots, you may want to override your compiler to
		the same version as used in [CI](.github/workflows/ci.yml), e.g.:

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

Are you sure you want to change the base?

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

Conversation

popzxc commented Jul 17, 2024 • edited Loading

popzxc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popzxc commented Jul 17, 2024

pksunkara commented Jul 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popzxc commented Jul 17, 2024

pksunkara commented Jul 18, 2024

popzxc commented Jul 18, 2024

pksunkara commented Jul 19, 2024

popzxc commented Jul 19, 2024

pksunkara commented Jul 19, 2024

popzxc commented Jul 24, 2024

pksunkara commented Aug 4, 2024

popzxc commented Aug 9, 2024 • edited Loading

pksunkara commented Aug 9, 2024

popzxc commented Aug 9, 2024

popzxc commented Sep 2, 2024

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

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

popzxc commented Jul 17, 2024 •

edited

Loading

popzxc commented Aug 9, 2024 •

edited

Loading