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

feat(cargo): support range=update-lockfile #23968

Closed
wants to merge 26 commits into from

Conversation

szlend
Copy link
Contributor

@szlend szlend commented Aug 20, 2023

Changes

Adds update-lockfile rangeStrategy support to the cargo manager.

Context

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@CLAassistant
Copy link

CLAassistant commented Aug 20, 2023

CLA assistant check
All committers have signed the CLA.

@szlend szlend changed the title cargo update-lockfile Draft: cargo update-lockfile Aug 20, 2023
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please sign the cla, otherwise we can't review

@rarkins rarkins changed the title Draft: cargo update-lockfile feat(cargo): support range=update-lockfile Aug 21, 2023
@szlend szlend force-pushed the cargo-update-lockfile branch 6 times, most recently from d8fb77a to 9f3fa85 Compare August 21, 2023 17:00
lib/modules/manager/cargo/artifacts.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/artifacts.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/extract.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/locked-version.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/types.ts Outdated Show resolved Hide resolved
@szlend
Copy link
Contributor Author

szlend commented Aug 21, 2023

Should I also update the default (auto) range strategy now that we support update-lockfile?

@viceice
Copy link
Member

viceice commented Aug 21, 2023

Should I also update the default (auto) range strategy now that we support update-lockfile?

yes and please don't force push. we squash merge, so no worry about git history

@szlend szlend marked this pull request as ready for review August 23, 2023 20:53
@rarkins rarkins requested a review from viceice August 24, 2023 07:23
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs more tests for coverage

lib/modules/manager/cargo/artifacts.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/artifacts.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/locked-version.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/range.ts Outdated Show resolved Hide resolved
@szlend szlend requested a review from viceice August 30, 2023 17:37
@szlend
Copy link
Contributor Author

szlend commented Oct 21, 2023

  • Added a hack to handle already updated dependencies. This is necessary when a preceding cargo command updates a later dependency.
  • Implemented updateLockedDependency which improved error reporting significantly.

I also tested this on a number of personal projects and haven't noticed any issues other than the previously mentioned Detected empty commit message in the Dashboard when something fails. Please re-review.

@szlend szlend requested a review from viceice October 21, 2023 13:11
Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's say that a PR already exists and is up-to-date. Will Renovate re-run cargo update commands every single time it runs on the repo/branch, or does it have a way of knowing that the locked version is already updated and no command is necessary?

@szlend
Copy link
Contributor Author

szlend commented Oct 23, 2023

Let's say that a PR already exists and is up-to-date. Will Renovate re-run cargo update commands every single time it runs on the repo/branch, or does it have a way of knowing that the locked version is already updated and no command is necessary?

Is this something each manager has to handle manually? I implemented the 'already-updated' case in updateLockedDependency so technically Renovate should be aware of the state of the lockfile at least.

Edit: If the question is referring to the hack I wrote to skip already updated dependencies, this is only used when a preceding command in a list of batched cargo update commands updates a later dependency in the batch. E.g. an update to serde-json also triggers an update to serde for whatever reason. Because we send a prebuild list of shell commands there's no other (simple) way to handle this that I could think of. I guess we could catch failures, parse stderr to figure out which dependencies are already up to date. Then retry updateArtifacts without those dependencies recursively until everything is updated. But that's FAR more complicated.

@rarkins
Copy link
Collaborator

rarkins commented Oct 23, 2023

I think you have it covered already with the already-updated part. You can verify that by running Renovate twice - first time to create the branch and second to check that no cargo update command was run

@szlend
Copy link
Contributor Author

szlend commented Oct 23, 2023

I think you have it covered already with the already-updated part. You can verify that by running Renovate twice - first time to create the branch and second to check that no cargo update command was run

The issue is within the same run. Between batched cargo update commands.

@szlend szlend requested a review from rarkins October 25, 2023 06:29
rarkins
rarkins previously approved these changes Oct 25, 2023
lib/modules/manager/cargo/artifacts.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/artifacts.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/artifacts.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/artifacts.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/artifacts.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/extract.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/extract.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/locked-version.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/locked-version.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/update-locked.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/artifacts.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/artifacts.ts Outdated Show resolved Hide resolved
lib/modules/manager/cargo/artifacts.ts Outdated Show resolved Hide resolved
@szlend szlend requested a review from viceice November 5, 2023 21:25
Comment on lines +69 to +77
export async function updateArtifacts(
{
packageFileName,
updatedDeps,
newPackageFileContent,
config,
}: UpdateArtifact,
recursionLimit = 10
): Promise<UpdateArtifactsResult[] | null> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please rename and make private, create a new updateArtifacts which called the renamed function, so recursion limit isn't part of the public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exactly how it's done in other managers, i.e. bundler. I'm really getting tired of this back and forth. Please be respectful of my time. I will leave some time for you to do a final review pass. Any other issues you can fix yourself. I'm happy to use this branch for my use case.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's best that we close this and someone else can pick it up to completion

@rarkins rarkins closed this Nov 6, 2023
@rarkins
Copy link
Collaborator

rarkins commented Nov 6, 2023

If anyone else has the time to pick this up and open an updated PR, please base it off @szlend's branch and let's make sure to name him as co-author in the final commit to main

@LunNova

This comment was marked as spam.

@baraknaveh

This comment has been minimized.

@rarkins

This comment was marked as resolved.

@baraknaveh

This comment has been minimized.

@renovatebot renovatebot locked and limited conversation to collaborators Nov 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support rangeStrategy=update-lockfile for Cargo
6 participants