Autopublishing to crates.io is broken

[davidlattimore]

Autopublishing has been broken since January. There were a few causes for this that have been fixed and at least one more to go. I probably should have raised an issue for this when I started looking into it, but better late than never. Some previous discussions on #14827 and pksunkara/cargo-workspaces#92

The remaining (known) issue causes publishing to fail with:

failed to select a version for the requirement `ra_ap_mbe = "=0.0.154"`
  candidate versions found which didn't match: 0.0.149, 0.0.148, 0.0.146, ...
  location searched: crates.io index
  required by package `ra_ap_cfg v0.0.154 (/home/runner/work/rust-analyzer/rust-analyzer/target/package/ra_ap_cfg-0.0.154)`
  perhaps a crate was updated and forgotten to be re-vendored?
error: unable to publish package ra_ap_cfg

i.e. it's trying to publish ra_ap_cfg, but can't because ra_ap_mbe hasn't been published yet. ra_ap_cfg has only a dev-dependency on ra_ap_mbe. We probably should just have the autopublish workflow remove all dev dependencies before it publishes. I'm currently looking for an existing tool to do that. If there isn't one, then perhaps we can add something to cargo-workspaces. @pksunkara, do you have any thoughts as to whether something like that might belong in cargo-workspaces?

[pksunkara]

Cargo ignores dev dependencies from the workspace because generally, they don't have a version and only have a path. This is how they used to look.

But with moving to workspace dependencies, the dev dependencies started to get versions too, and Cargo errors on them.

One way we can fix this is to make sure we also account for dev dependencies when building the DAG, but:

1. I don't know if that will lead to a circular graph since I don't know if Rust allows it.
2. Need to make sure private crates are properly handled and not published.

Or delete the dev dependencies as you mentioned. Honestly, Cargo should be ignoring them during publishing, but until then I don't mind adding a workaround.

[Veykril]

cargo allows having circular dev dependencies unfortunately (you can even just point a dev dependency on the package itself ...)

[davidlattimore]

We're making it slightly further now. ra_ap_cfg published successfully. Now we fail with:

Caused by:
  the remote server responded with an error: this crate exists but you don't seem to be an owner. If you believe this is a mistake, perhaps you need to accept an invitation to be an owner before publishing.
error: unable to publish package ra_ap_la-arena

It looks like @matklad is the only owner of ra_ap_la-arena, but we need rust-lang-owner added as an owner, since that seems to be what the autopublish workflow runs as.

[davidlattimore]

List of packages that probably need ownership fixes:

ra_ap_la-arena
ra_ap_intern
ra_ap_line-index
ra_ap_proc-macro-srv-cli
ra_ap_lsp-server

[Veykril]

right, intern and proc-macro-srv-cli were created with matklad's key after the move, hence why the account owner has not been updated for those. The other 3 ideally shouldn't be published like that at all since we want those to be proper crates.io libraries.

[matklad]

Fixed owners for intern and macro-cli. Agree that others shouldn’t exist

[davidlattimore]

For the three other crates (lsp-server, la-arena, line-index), how do we want publishing to work? Options that come to mind:

1. We require that any time they're changed, the versions in Cargo.toml are bumped appropriately, then each week we automatically publish them if they've changed.
2. We use some tooling to try to automatically determine whether a breaking change has been made and make the appropriate version bump then automatically publish.
3. We manually publish as needed each time we find that the main rust-analyzer crates no longer compile with the versions of these libraries on crates.io.
4. We manually publish as needed, but change rust-analyzer crates to always use the versions from crates.io. This would mean that when new APIs are added to these crates, they'd need to be published before any change in rust-analyzer could use the new APIs. At that point, it might make sense to move these crates out into separate repositories.

My concern with option 3 would be that automatic publishes may break frequently when APIs are added/changed in these library crates if they don't get published before the autopublish workflow runs. It would also mean that the rust-analyzer release binaries would frequently be using different versions of these crates than what's available on crates.io.

[pksunkara]

I think 4 is the right approach of managing dependencies.

At that point, it might make sense to move these crates out into separate repositories.

We don't really need to do that though. icu4x keeps yoke in the repository itself but publishes it independently.

[Veykril]

We should not do timed releases for those I think, they shouldn't change too often. I think we should go for an automated publish if the version in the cargo toml has been adjusted. (Effectively allowing PRs to publish the crate which means its still manual but with less churn). The main question is how we can make sure that the r-a crates don't depend on local only changes as those might slip through reviews unnoticed which will result in publish failure as you've mentioned.

So in a sense I'd like a mix between 3 and 4

[matklad]

Yup, "publish upon merge into master if version in Cargo.toml is different" is the way to go.

The main question is how we can make sure that the r-a crates don't depend on local only changes

We should only depend on crates.io versions of the crates anywhere. That means that changing these crates requires a chain of two PRs, but that's Ok; that's exactly the workflow one would use if these crates were in a separate repo.

To make this easier, we should pull all crates to top-level Cargo.toml, and use only .workspace = true dependencies, ensuring that there's only one place to patch dependencies.

[matklad]

I'd also prefer the publishing for libs crate to not go via cargo-workspaces. Unlike ap crates, we actually do want to provide guarantees wrt to libs publishing, and that means we should write first-party integration in an xtask, rather than rely on outside tools. The code for that can be stolen from here:

https://github.com/matklad/once_cell/blob/master/xtask/src/main.rs#L80-L100

Not sure if we should keep "does the tag exist" logic, or forgo tags and just check if the crate exists on crates.io, either works fine for me.

[pksunkara]

we actually do want to provide guarantees wrt to libs publishing

But isn't that guarantee defined by the version bump in PRs? Then cargo-workspaces can do just publishing with --from-git.

[matklad]

To provide guarantees that the stuff works we need:

• either rely on strong guarantees provided by first-parties (Rust toolchain itself, GitHub)
• or own the relevant code

CI and builds are notoriously fiddly, anything we want to guarantee to work shouldn’t use any external dependencies.

[davidlattimore]

Looks like I missed a couple of crates when I was checking crates.io ownership. Besides ra_ap_intern, which @matklad sent an invite for rust-lang-owner already, there's also ra_ap_proc-macro-srv-cli

[matklad]

Hm, I think I’ve send an invite for that as well, and I also don’t see a new owner for the intern crate, although invite for that went out a week ago:

https://crates.io/crates/ra_ap_intern

perhaps rust-lang-owner doesn’t pick invites?

[lnicola]

Failed because of a hyphens/underscores mismatch in https://github.com/rust-lang/rust-analyzer/actions/runs/5432810012/jobs/9880094469.

[davidlattimore]

As far as I can tell, the proc_macro_test crate on crates.io is unrelated to the one in rust-analyzer repo. The one in the rust-analyzer repo is set to publish = false. It's only used as a dev-dependency of proc_macro_srv, so I would have thought the change I made to cargo-workspaces to remove dev-dependencies before publishing would have removed it.

[davidlattimore]

It looks like it's due to this commit to cargo-workspaces. It now won't remove dev dependencies when publishing unless at least one of the dev-dependencies is a package that we're going to publish - at least that's my understanding.

In this case ra_ap_proc_macro_srv has a dev-dependency on proc-macro-test, however because proc-macro-test is set to publish = false, it's not considered. @pksunkara any suggestions?

[pksunkara]

The correct way to fix this would be to remove version from workspace.dependencies (this line). Any workspace crates that are publish = false should not have version field when being used.

[lnicola]

Finally worked in https://github.com/rust-lang/rust-analyzer/actions/runs/5437937695.

[davidlattimore]

Woohoo! Thanks for your patience and help everyone :)

[regexident]

As a dependent of ra_ap_rust-analyzer and a bunch of its sibling crates: THANK YOU SO MUCH FOR FIXING THIS! 🙏

[lnicola]

Failed again in https://github.com/rust-lang/rust-analyzer/actions/runs/5504372831/jobs/10030522070.

[davidlattimore]

Looks like the relevant line is:

warning: timed out waiting for `ra_ap_vfs v0.0.161` to be available in registry `crates-io`

It then subsequently failed while trying to publish ra_ap_base_db v0.0.161 because ra_ap_vfs v0.0.161 wasn't available yet (it is now).

The timeout warnings looks to have come from cargo, which has a 60 second timeout.

What I don't understand, is why cargo-workspaces didn't then wait for the new version to be available. It has a timeout of 300 seconds for the version to appear in the index, but as far as I can see from the logs, it never printed out "waiting...", which makes it look as though it found the new version of the first check.

[pksunkara]

Your evaulation is correct. Could this be an issue with sparse registry or other index changes with cargo?

[lnicola]

Worked after a small fix and a retry: https://github.com/rust-lang/rust-analyzer/actions/runs/5507544712.