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.

Sign up for GitHub

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

Jump to bottom

[WIP] feat!: bump workspace dependencies with release #17

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

[WIP] feat!: bump workspace dependencies with release #17

wants to merge 2 commits into from

Conversation

brooksmtownsend
Copy link

Fixes #8

This PR adds support for bumping workspace dependencies just like how the current smart-releaser will bump dependent crates in response to a new release. In order to implement this, I just slotted some logic in with the existing logic to find dependent crates to also look for a workspace.dependencies section in the Cargo.toml.

This works... mostly. It actually works perfectly for dependencies that the main workspace uses, but it does not work when a dependency is specified at the workspace level but not used by the main workspace.

For example, I was taking a look at this for wasmCloud. If I use this to bump the crate wasmcloud-core, which our primary package depends on, I get the expected result (wasmcloud-core bumps from 0.5 to 0.6, workspace dependency updates. However, if I use this to bump wash-lib, this is specified as a workspace dependency but wasmCloud itself doesn't use it, so no update. I wanted to open up this PR as a draft WIP while I work through this just to let you know I'm working on it 😄

Some trace logs from a run (will remove extraneous trace logs when I mark this as ready of course):

[TRACE] Updating wasmcloud
[TRACE] Crates that may change wasmcloud-core, opentelemetry-nats, provider-archive, wasmcloud-runtime, wasmcloud-tracing, wasmcloud-host, wasmcloud-provider-sdk, wasmcloud-provider-blobstor
e-fs, wasmcloud-provider-blobstore-s3, wasmcloud-provider-http-client, wasmcloud-provider-http-server, wasmcloud-provider-keyvalue-redis, wasmcloud-provider-keyvalue-vault, wasmcloud-provide
r-lattice-controller, wasmcloud-provider-messaging-kafka, wasmcloud-provider-messaging-nats, wasmcloud, wash-cli, wash-lib
[TRACE] Found workspace dependencies
[TRACE] Pending 'wasmcloud' manifest workspace-dependencies update: 'wasmcloud-core = "^0.6.0"' (from  "0.5")
[TRACE] Pending 'wasmcloud' conservative manifest workspace-dependencies update: 'provider-archive = "^0.10.1"' (from  "0.10")
[TRACE] Pending 'wasmcloud' manifest workspace-dependencies update: 'wasmcloud-tracing = "^0.4.0"' (from  "0.3")
[TRACE] Pending 'wasmcloud' manifest workspace-dependencies update: 'wasmcloud-provider-sdk = "^0.5.0"' (from  "0.4")

@brooksmtownsend brooksmtownsend marked this pull request as draft April 25, 2024 16:31
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 25, 2024
View reviewed changes
src/command/release/manifest.rs Fixed Show resolved Hide resolved
src/command/release/manifest.rs Fixed Show fixed Hide fixed
@brooksmtownsend brooksmtownsend force-pushed the feat/update-workspace-dependencies branch from 4ccae70 to 6902c4e Compare April 25, 2024 16:34
@brooksmtownsend brooksmtownsend force-pushed the feat/update-workspace-dependencies branch from 6902c4e to e09daf1 Compare April 25, 2024 16:35
@brooksmtownsend
Copy link
Author

I think test is failing because I added extra output.

Thinking out loud, I think I need to consider a workspace root as something that may always change in response to a version bump, and that'll work well to catch my edge case

@Byron
Copy link
Owner

Byron commented Apr 25, 2024

Thanks a lot for getting the ball rolling on workspace dependency support!

I thought that maybe it's worth it to try to create a new journey tests that exercises this new capability as well.

The biggest issue I see with any change is that it might very well break gitoxide and I'd only know when it's too late. So I suppose I could use this PR for a while and if it works for both of us, it can be merged 😅.

@brooksmtownsend
Copy link
Author

brooksmtownsend commented Apr 26, 2024 •
edited
Loading

@Byron what kind of tests could we add to add confidence in not breaking gitoxide? I'd be happy to guinea pig test this release, though I can only speak to a few use cases 😕

edit: I totally missed that gitoxide is a user of this binary, so that makes sense that we don't want to break it 😄

@Byron
Copy link
Owner

Byron commented Apr 26, 2024

I don't think given this requirement, there isn't anything less but to publish gitoxide itself, and such a publish can come in many different shapes. Impossible to model these. On the bright side, that's the only requirement, so if I manage to create one release with a version from this PR, I'd assume it's not breaking anything.

But like a said, a simple Journey test should probably be added to generally show how it works with workspace dependencies.

@brooksmtownsend brooksmtownsend mentioned this pull request Apr 29, 2024
Merged
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 30, 2024
View reviewed changes
src/command/release/manifest.rs Fixed Show fixed Hide fixed
src/command/release/manifest.rs Fixed Show fixed Hide fixed
src/command/release/manifest.rs Fixed Show fixed Hide fixed
src/command/release/manifest.rs Fixed Show fixed Hide fixed
@brooksmtownsend
wip: working workspace
2065c01 
Signed-off-by: Brooks Townsend <[email protected]>
@brooksmtownsend brooksmtownsend force-pushed the feat/update-workspace-dependencies branch from 466c458 to 2065c01 Compare April 30, 2024 21:00
@brooksmtownsend
Copy link
Author

Just a note my latest commit isn't quite ready for code review, but it does work! There's some edge cases I have called out in the TODO, and more to test, but I'm planning on trying it out to release a few crates this week (likely via manual inspection)

@Byron Byron mentioned this pull request May 1, 2024
Open
This was referenced May 6, 2024
Merged
Closed
@Byron
Copy link
Owner

Byron commented May 22, 2024

By the way, I tried to publish with this branch but it failed with an issue around invalid manifests being written. cargo metadata failed due to that. I am sorry to not have the specific output, I forgot to save it while it was still available in the terminals history.

@brooksmtownsend
Copy link
Author

Thank you for letting me know @Byron , I think an issue here (with my implementation) is still that

  • smart-release with workspace dependencies still edits manifest without --execute

If that wasn't the issue though, let me know what the error is and I can try to track it down 🙂

@Byron
Copy link
Owner

Byron commented May 22, 2024

Thank you! I think it should be possible to reproduce the issue if the gitoxide main branch is reset to 9511416a6cd0c571233f958c165329c8705c2498. Then, cargo smart-release will do strange things, I think it does indeed change a manifest (maybe too early), but in a way that fails cargo metadata invocations.

@brooksmtownsend
Copy link
Author

Yeah I bet if you have any pre-check step then the above bug will cause an issue, e.g. run without --execute to double check, then run with --execute

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cargo-smart-release doesn't update [workspace.dependencies] version
2 participants
@brooksmtownsend @Byron