Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
nahsi opened this issue Apr 3, 2023 · 7 comments
Open

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

nahsi opened this issue Apr 3, 2023 · 7 comments
Assignees
Labels
help wanted We'd love to have community involvement on this issue. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@nahsi
Copy link

nahsi commented Apr 3, 2023

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.

@nahsi nahsi added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Apr 3, 2023
@chingor13
Copy link
Contributor

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.

@chingor13 chingor13 added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. help wanted We'd love to have community involvement on this issue. and removed priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Apr 10, 2023
@cdata
Copy link
Contributor

cdata commented May 6, 2023

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

@cdata
Copy link
Contributor

cdata commented May 6, 2023

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
Copy link
Contributor

cdata commented May 7, 2023

@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
Copy link
Contributor

cdata commented May 8, 2023

🚨 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: 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
Copy link
Contributor

cdata commented May 8, 2023

I was able to temporarily patch package.json to get around this per the suggestion from @ben-foxmoore here: #1945 (comment)

@cdata
Copy link
Contributor

cdata commented May 8, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted We'd love to have community involvement on this issue. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

3 participants