-
Notifications
You must be signed in to change notification settings - Fork 1k
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
bump corepack to fix freezes #11126
bump corepack to fix freezes #11126
Conversation
Some interesting Corepack related failures in the Specs. I don't have the time to track those down. Anyone is welcome to pick this up before I can get to it. |
@jeffwidman nice, seems like there's now multiple reasons to bump Corepack! |
@@ -71,6 +72,18 @@ def initialize(version) | |||
super(T.must(version)) | |||
end | |||
|
|||
sig { params(version: VersionParameter).returns(VersionParameter) } | |||
def clean_version(version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Tip:
Corepack's newer versions, when installing dependencies locally, may attempt to add the packageManager
field to the package.json
. Instead of returning the expected version, Corepack sometimes outputs a warning message that includes the version, leading to a malformed version string. This method cleans the version string by extracting the correct semantic version (x.y.z
) from the warning or input, ensuring compatibility with Dependabot and related processes.
Yes. When corepack installing locally it is expecting to have packageManager field and returning warning instead of the version. So I am cleaning up the version to return version instead of the warning which is causing malformed version error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the updates! The changes look good overall. It would be great to confirm that everything works as expected with the new version handling logic.
@@ -62,6 +62,7 @@ def self.semver_for(version) | |||
|
|||
sig { override.params(version: VersionParameter).void } | |||
def initialize(version) | |||
version = clean_version(version) | |||
@version_string = T.let(version.to_s, String) | |||
version = version.gsub(/^v/, "") if version.is_a?(String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line 67 is also cleaning a version string; can we consolidate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is also moved.
What are you trying to accomplish?
There was some kind of freeze in Corepack that was reproducible if you gave it a registry that would 404:
☝️ The process would not exit from there. It just hangs.
I noticed our version was out of date, so bumping it fixed the problem.
Anything you want to highlight for special attention from reviewers?
How will you know you've accomplished your goal?
We should be able to also verify that it fixes the issue in production with a live test.
Checklist