search

rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.76k stars 2.42k forks source link

`-Zpackage-workspace` is not smart about `publish = false` #14356

Closed workingjubilee closed 2 months ago

workingjubilee commented 3 months ago

Problem

I was hoping to include -Zpackage-workspace as part of a workspace's CI so that it doesn't repeat certain embarrassing mistakes, but it didn't work as expected with respect to certain crates that have publish = false on them.

Steps

After bumping the versions of all packages I wished to publish a new release for, I used this on the workspace with the following command:

$ cargo +nightly package --workspace -Zpackage-workspace
     Locking 7 packages to latest compatible versions
    Updating cargo-pgrx v0.12.0-beta.3 (/home/jubilee/tcdi/pgrx/cargo-pgrx) -> v0.12.0-beta.4
    Updating pgrx v0.12.0-beta.3 (/home/jubilee/tcdi/pgrx/pgrx) -> v0.12.0-beta.4
    Updating pgrx-macros v0.12.0-beta.3 (/home/jubilee/tcdi/pgrx/pgrx-macros) -> v0.12.0-beta.4
    Updating pgrx-pg-config v0.12.0-beta.3 (/home/jubilee/tcdi/pgrx/pgrx-pg-config) -> v0.12.0-beta.4
    Updating pgrx-pg-sys v0.12.0-beta.3 (/home/jubilee/tcdi/pgrx/pgrx-pg-sys) -> v0.12.0-beta.4
    Updating pgrx-sql-entity-graph v0.12.0-beta.3 (/home/jubilee/tcdi/pgrx/pgrx-sql-entity-graph) -> v0.12.0-beta.4
    Updating pgrx-tests v0.12.0-beta.3 (/home/jubilee/tcdi/pgrx/pgrx-tests) -> v0.12.0-beta.4
error: `aggregate` cannot be packaged.
The registry `crates-io` is not listed in the `package.publish` value in Cargo.toml.

Possible Solution(s)

This seems a curious error, given that this is intentional? I wish to not publish that crate and thus do not wish to package it for publishing?

Cargo has the option of doing the "smart" thing ("What I mean") here and simply ignoring such packages. The other obvious alternative is that it can simply continue to fail-fast, but if so it should probably have a more helpful error message. Note these are "leaf" packages on which nothing depends, so automatically excluding them would work.

Notes

I do have the option of simply removing it from the workspace, but it all gets a bit awkward. To complicate matters further, these packages being in the workspace does affect feature resolution which also affects whether or not basic cargo check or cargo test work (as the library is one of those "you must enable one and only one of these features" sorts).

Related:

Version

cargo 1.82.0-nightly (b5d44db1d 2024-07-26)
release: 1.82.0-nightly
commit-hash: b5d44db1daf0469b227a6211b987162a39a54730
commit-date: 2024-07-26
host: x86_64-unknown-linux-gnu
libgit2: 1.8.1 (sys:0.19.0 vendored)
libcurl: 8.8.0-DEV (sys:0.4.73+curl-8.8.0 vendored ssl:OpenSSL/3.3.1)
ssl: OpenSSL 3.3.1 4 Jun 2024
os: Arch Linux Rolling Release [64-bit]
torhovland commented 3 months ago

I agree this looks like an issue. However, I guess it's a matter of interpretation whether or not publish = false implies it shouldn't be packaged. In the example above, "doing what I mean" implies not packaging it. But are there other use cases where you might want to package but not publish?

epage commented 3 months ago

Because publish = false allows cargo package to run today, we would need to maintain that. So in this case, cargo +nightly package --workspace -Zpackage-workspace should succeed with every workspace member packaged.

torhovland commented 3 months ago

should succeed with every workspace member packaged

Every member except for those that cannot be packaged (hence may have publish = false), right?

epage commented 3 months ago

No, that doesn't follow with what I said.

From what I can tell, cargo package can run when package.publish = false. So if a user selects a package to run cargo package on, whether via the CWD, with -p, or with --workspace, we should package it, independent of package.publish.

torhovland commented 3 months ago

OK, so packaging should simply not fail on:

The registry `crates-io` is not listed in the `package.publish` value in Cargo.toml.
epage commented 3 months ago

As far as I can tell from existing precedence in cargo package, yes. Which might put a wrinkle in how we did workspace publishing packaging.

workingjubilee commented 3 months ago

...I uhh... certainly would not want "publish the publish.false crates" to become the semantic of cargo publish --workspace if that ever appears. 😅

workingjubilee commented 3 months ago

I do have the option of simply removing it from the workspace, but it all gets a bit awkward. To complicate matters further, these packages being in the workspace does affect feature resolution which also affects whether or not basic cargo check or cargo test work (as the library is one of those "you must enable one and only one of these features" sorts).

It looks like the thing that I would most want to do in the meantime is to cull the workspace down to only the crates I want to publish, but I can't think of a good way, currently, to enable a feature for some of the packages in the workspace so that cargo check and cargo test still work and simply use the default feature I want them to.

epage commented 3 months ago

I wonder if that would be a flag of interested for cargo hack, a --publishable or something. Would be a good way to vet use cases and usefulness to do it in a third-party command.

torhovland commented 3 months ago

I'll try writing a test for this.

torhovland commented 3 months ago

@epage @jneem While this issue was quite easy to fix, I'm not sure it's the right fix. Could you take a look at my description in https://github.com/rust-lang/cargo/pull/14364, please?

workingjubilee commented 3 months ago

typo whoops.

workingjubilee commented 2 months ago

I carried out the repo refactoring I mentioned. Obviously, what I "really want" is cargo publsih --workspace --dry-run, but that doesn't exist today. That day seems like a long way off. Even if it suddenly exists, and works as expected, it seemed likely I would be frustrated by it for other reasons with my current half-published half-not workspace. Thus the refactoring seemed useful, even if some details wound up quite gnarly.

For now, I am quite pleased that cargo package --workspace (actually cargo +nightly package --workspace -Zpackage-workspace but whatever) seems to now do exactly what I want it to do with this feature, as for a while now I have not had confidence that the pgrx repository can be released. This is both because of the whole "publishing an entire workspace isn't an atomic step" thing, and also because some changes in Cargo.toml can break packaged code while not affecting the usual tests, thus creating the oddity of code that passes CI but does not work in its actual release. If I don't pay close attention and notice that e.g. the docs.rs build is broken (which is a very indirect sign!), I can miss that happening. I want to do more repository-level refactoring, so having this in-place, and it being so easy... I thought I'd have to write my own non-integrated tooling, which is necessarily more verbose, and thus a slog... really helps.

epage commented 2 months ago

That day seems like a long way off

The reason we have cargo package --workspace is people are actively working towards cargo publish --workspace.

Even if it suddenly exists, and works as expected, it seemed likely I would be frustrated by it for other reasons with my current half-published half-not workspace. Thus the refactoring seemed useful, even if https://github.com/pgcentralfoundation/pgrx/pull/1796.

I assume what you are referring to is cargo publish --dry-run --workspace skipping publish = false packages. The semantics of cargo publish and cargo package are different which carries over when --workspace is used with cargo publish --workspace likely being less well constricted, allowing us to more openly discuss ideas like this.

workingjubilee commented 2 months ago

"a long way off" can still be far enough out that it's worth the time to reengineer my repo to make things work now!

I assume what you are referring to is cargo publish --dry-run --workspace skipping publish = false packages. The semantics of cargo publish and cargo package are different which carries over when --workspace is used with cargo publish --workspace likely being less well constricted, allowing us to more openly discuss ideas like this.

oh good.