googleapis / release-please

generate release PRs based on the conventionalcommits.org spec
https://www.conventionalcommits.org
Apache License 2.0
4.64k stars 350 forks source link

cargo-workspace plugin ignores changes in `workspace.dependencies` #1896

Open nahsi opened 1 year ago

nahsi commented 1 year ago

We are using release-please github action in manifest mode with cargo workspace plugin.

Repository is using cargo workspaces. Some of dependencies are set in root Cargo.toml file in [workspace.dependencies].

Update to a dependency with correct prefix should trigger release of all crates that use that dependency, right? But it doesn't.

This PR didn't trigger any changes.

chingor13 commented 1 year ago

The cargo-workspace plugin currently only builds the dependency tree from the [workspace] definition that declares the modules in the repo and the [dependencies]/[dev-dependencies] sections declared in those modules' Cargo.toml files.

Personally, I am unfamiliar with the full capabilities of Cargo for rust packages and this plugin was created by the community. If this [workspace.dependencies] is a legitimate way of declaring extra dependencies, then we'd love a community contribution to augment the building of the dependency tree.

cdata commented 1 year ago

Just wanted to chime in to say we just hit this. I'll take a look at possibly making a contribution to fix.

cdata commented 1 year ago

Just for context, [workspace.dependencies] is more sophisticated than just "extra" dependencies. It allows you to specify a global dependency version with optional default features for that dependency. Then, a workspace member crate can inherit that workspace root dependency and overwrite the default features if needed. Relevant documentation here.

So in order to factor this in, we need to check for dependencies in a crate's manifest that have workspace = true set. If present, the dependency is inherited from the workspace root manifest, so changes at the root level will determine whether or not the member crate's dependency has changed.

cdata commented 1 year ago

@chingor13 there is a code path in the rust strategy that appears to handle a workspace with multiple crates. However, this seems to be distinct from (and perhaps unrelated to) the cargo-workspace manifest plugin. We don't seem to hit that code path in our release process, for example. Do you have any context for this? Is this code path perhaps a deprecated/disused strategy for handling cargo workspaces?

cdata commented 1 year ago

:rotating_light: wow, it looks like there is some kind of NPM supply chain crisis related to Octokit going on right now. Builds on this repo are failing, and I actually can't compile my patch to test if it works.

Some related issues in the Octokit supply chain:

This appears to be impacting the CI jobs on this project as well.

Anyway, I have a WIP but as-yet-untested patch going here: https://github.com/googleapis/release-please/compare/main...cdata:release-please:fix/support-cargo-workspace-dependency-inheritance

I'll check back in a few days and hopefully release-please will be buildable at that time. If you have any tips for building it under the current NPM supply chain conditions, please share!

cdata commented 1 year ago

I was able to temporarily patch package.json to get around this per the suggestion from @ben-foxmoore here: https://github.com/googleapis/release-please/issues/1945#issuecomment-1538616889

cdata commented 1 year ago

My patch isn't cleaned up yet, but as a proof of concept I believe it is working. Going to be deploying it on our project to test it out.

Just a note that at this time it supports detecting when a workspace-inherited dependency version has changed in a way that impacts a workspace member, but it doesn't yet support detecting when the default feature flags change for an inherited dependency (and this seems a little bit in the grey area as far as versioning is concerned).