Sort across all workspace deps (normal, dev & build)

[Voxelot]

This PR updates the action to support ordering across all categories of dependencies, rather than just normal dependencies like before. Now if you have crates in your workspace used for integration testing or proc-macros at build time, they will publish in the correct order before their dependents.

closes #305

[Voxelot]

@katyo This is my first foray into github actions, so if there's anything else I need to do for testing let me know.

This is currently a blocker for us at Fuel Labs so I'm happy to make any changes needed to get a quick turnaround: https://github.com/FuelLabs/fuels-rs/actions/runs/1759190798

[katyo]

@Voxelot Thank you for this pr.

How are you handle situation with circular dependencies? The concrete example rquickjs crate.
The rquickjs crate depends from rquickjs-macro in turn the rquickjs-macro has rquickjs as a dev-dependency.

[Voxelot]

@katyo It seems like the only way to publish a set of crates with circular dependencies is to pass the --no-verify flag. How about disabling cyclic checking if --no-verify is set?

[katyo]

@Voxelot I'm not quite familiar with cargo internals. Does cargo handles dev-dependencies in same way as dependencies and build-dependencies in particular when publishing crates?
I don't sure but may be we need a config option to disable checking internal (local) crates listed in dev-dependencies.

[Voxelot]

Based on my testing, it appears cargo applies the same cyclic detection to dev-dependencies as normal and build dependencies during a publish. I'm not sure why the decision was made to require dev-dependecies to be available during publish (maybe for rust-doc example code or license validation). I found an outstanding issue about this from a few years ago but it doesn't look like much progress has been made since it seems complicated to satisfy the requirement to publish all dev-dependencies: rust-lang/cargo#4242

Apart from disabling cyclic checking with the --no-verify flag, this PR behaves the same way as Cargo.

[Voxelot]

After some additional investigation, I discovered that dev-dependencies aren't treated like normal dependencies during publish validation if they use local paths instead of versions.

https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#development-dependencies

Note: When a package is published, only dev-dependencies that specify a version will be included in the published crate. For most use cases, dev-dependencies are not needed when published, though some users (like OS packagers) may want to run tests within a crate, so providing a version if possible can still be beneficial.

I've updated the PR to reflect this behavior. Now dev-dependencies without a version aren't included in cyclic checking.

It appears rquickjs includes local dev-deps by paths or version interchangeably (ex1 vs ex2). Since it may not be possible to only use local paths for dev-dependencies in all cases (e.g. rust-doc code examples), I've also added the no-verify configuration option. This will bypass cyclic detection and also pass the --no-verify flag to cargo publish.

[katyo]

Never?

[katyo]

This action checks version availability and tries to download package of corresponding version for each dependency.

[katyo]

I think that complete bypassing sorting isn't a best solution. It may cause problems with publishing (for ex. in case of rquickjs). In my opinion skipping dev-dependencies when sorting is much better.

[Voxelot]

If verification is disabled theoretically the order is irrelevant since the deps are only downloaded and checked during cargo publish packaging (which is skipped when --no-verify is used).

[katyo]

Course, but in that case no-verify will be required to publish rquickjs.

Actually --no-verify arg is used here to speed-up publishing by skipping complex verification because it already executed in previous steps of CI workflow.

[katyo]

After some additional investigation, I discovered that dev-dependencies aren't treated like normal dependencies during publish validation if they use local paths instead of versions.

https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#development-dependencies

Note: When a package is published, only dev-dependencies that specify a version will be included in the published crate. For most use cases, dev-dependencies are not needed when published, though some users (like OS packagers) may want to run tests within a crate, so providing a version if possible can still be beneficial.

I've updated the PR to reflect this behavior. Now dev-dependencies without a version aren't included in cyclic checking.

It appears rquickjs includes local dev-deps by paths or version interchangeably (ex1 vs ex2). Since it may not be possible to only use local paths for dev-dependencies in all cases (e.g. rust-doc code examples), I've also added the no-verify configuration option. This will bypass cyclic detection and also pass the --no-verify flag to cargo publish.

In my opinion we could simply skip dev-dependencies with specified path even if version is set too. Adding no-verify option is not necessary in such case.

[Voxelot]

In my opinion we could simply skip dev-dependencies with specified path even if version is set too. Adding no-verify option is not necessary in such case.

We can't ignore versioned dev-deps from the publishing order without using the --no-verify flag. As I already mentioned before, with versions set

cargo applies the same cyclic detection to dev-dependencies as normal and build dependencies during a publish

Not only are cyclic dependencies an issue, but the packaging verification during a normal cargo publish will attempt to download any versioned dependencies, regardless of whether they are normal, build or dev deps. This means dev-deps with a version in a workspace need to be sorted and published in the correct order. The only way around this issue is to disable packaging verification with --no-verify, which is not an acceptable default for most users.

The reasoning for this is documented here:

• If a [dev-dependency] has a version, it behaves exactly as today. The path is dropped, it's verified to exist on crates.io, and it's published to crates.io with the dependency.
• If a [dev-dependency] does not have version (and is path) then it's removed from the manifest that crates.io knows about during publication. This means Cargo/crates.io would silently allow dev-dependencies without version markers when publishing.'

rust-lang/crates.io#1789 (comment)

It seems like the main usecase for publishing dev-deps was for linux distros to be able to run cargo test on third party published crates, but it's kind of a strange use-case. Regardless, cargo / crates.io currently supports that still, so versioned dev-deps must be published.

An alternative which might be a more reasonable default than --no-verify, is to strip all dev-deps from all the workspace Cargo.toml files before publishing, and then use cargo publish --allow-dirty.