Skip to content

Update dependencies to fix panic in go generate#326

Open
mat007 wants to merge 2 commits into
microsoft:mainfrom
mat007:fix-go-generate
Open

Update dependencies to fix panic in go generate#326
mat007 wants to merge 2 commits into
microsoft:mainfrom
mat007:fix-go-generate

Conversation

@mat007

@mat007 mat007 commented Jan 30, 2025

Copy link
Copy Markdown

I picked Go 1.22 because I’ve seen this in another recent PR run:

Setup go version spec oldstable
oldstable version resolved as 1.22.10

Also updating golang.org/x/tools/cmd/stringer requires Go 1.22:

$ GOTOOLCHAIN=go1.21.0 go get golang.org/x/tools/cmd/stringer
go: golang.org/x/tools/cmd/stringer: golang.org/x/[email protected] requires go >= 1.22.0 (running go 1.21.0; GOTOOLCHAIN=go1.21.0)

Here is what I did:

GOTOOLCHAIN=go1.22.0 go get golang.org/x/tools/cmd/stringer
GOTOOLCHAIN=go1.22.0 go mod tidy

then manually edited go.mod to remove the added toolchain, and re-ran go mod tidy to make sure it’s stable.

@mat007
mat007 requested a review from a team as a code owner January 30, 2025 08:14
This fixes the crash when go generating

Signed-off-by: Mathieu Champlon <[email protected]>
@mat007 mat007 changed the title Update dependencies to fix go generate crash Update dependencies to fix panic in go generate Jan 30, 2025
@mat007 mat007 mentioned this pull request Jan 30, 2025
kiashok
kiashok previously approved these changes Jan 30, 2025

@kiashok kiashok left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM, thanks!

Comment thread go.mod Outdated
go 1.21
go 1.22.0

toolchain go1.22.10

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can probably remove this one if it automatically added it (go modules tends to randomly add these, but adding also means that users get either down- or upgraded when running commands, so I try to avoid adding these.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, I’ll remove the toolchain.

Comment thread go.mod
module github.com/Microsoft/go-winio

go 1.21
go 1.22.0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did the updated modules enforce updating to go1.22.0, or does it accept staying on go1.21 ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, it needs go 1.22:

$ GOTOOLCHAIN=go1.21.0 go get golang.org/x/tools/cmd/stringer
go: golang.org/x/tools/cmd/stringer: golang.org/x/[email protected] requires go >= 1.22.0 (running go 1.21.0; GOTOOLCHAIN=go1.21.0)

Signed-off-by: Mathieu Champlon <[email protected]>
@mat007

mat007 commented Feb 11, 2025

Copy link
Copy Markdown
Author

@kiashok can you trigger the CI again?

@mat007

mat007 commented Feb 11, 2025

Copy link
Copy Markdown
Author

All right, so this effectively fixes the «CI / Go Generate» workflow.
Do you folks want to merge this PR as is, or should we cherry-pick this to #325 to fix all workflows in a single PR?

@kiashok

kiashok commented Feb 24, 2025

Copy link
Copy Markdown

All right, so this effectively fixes the «CI / Go Generate» workflow. Do you folks want to merge this PR as is, or should we cherry-pick this to #325 to fix all workflows in a single PR?

@mat007 #325 might not fix all the CI issues being hit here right? The interger overflow ones especially - do you want me to look into the integer overflow lint errors? Apologies that a lot of this has fallen on you.

@mat007

mat007 commented Feb 25, 2025

Copy link
Copy Markdown
Author

@mat007 #325 might not fix all the CI issues being hit here right? The interger overflow ones especially - do you want me to look into the integer overflow lint errors? Apologies that a lot of this has fallen on you.

Thanks @kiashok ! I can look at the lint errors in that other PR, but I need some guidance on how best to approach it, see #325 (comment)

But also, do we want to merge here, or cherry-pick the commits to #325? I really don’t mind either way. 😄

@mat007

mat007 commented Aug 26, 2025

Copy link
Copy Markdown
Author

@kiashok sorry for pinging you again.
Is there any chance we can merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants