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

chore: remove patch version from go version in go.mod #88

Closed
wants to merge 1 commit into from

Conversation

crazy-max
Copy link
Contributor

relates to #57 (comment)

@ldez
Copy link
Contributor

ldez commented Jul 3, 2024

Hello,

technically speaking, the patch part of the version is now a kind of requirement, there is some tolerance for compatibility reasons.

I don't remember where I saw the explanation, I will find it.

@ldez
Copy link
Contributor

ldez commented Jul 3, 2024

🤔 I didn't find where I saw that, maybe I made a confusion with toolchain.

Why is this important for you?

the go version is the minimal Go version so go1.21 and go1.21.0 are the same thing.

@crazy-max
Copy link
Contributor Author

crazy-max commented Jul 3, 2024

Why is this important for you?

the go version is the minimal Go version so go1.21 and go1.21.0 are the same thing.

It means downstream projects need to set same go version 1.21.0 in go.mod even if 1.21 is still correct. My change is just to avoid this kind of noise if that makes sense.

@crazy-max
Copy link
Contributor Author

technically speaking, the patch part of the version is now a kind of requirement,

I guess since 1.22 correct?

@cameracker
Copy link

I dont see anything in the documentation that implies youre forced to use 1.21.0 and cant go to 1.21.1. Everything in the go mod docs indicate that this is the minimum go version required for this package and consumers are free to go to a later version as needed.

Is there an error youre observing?

@ldez
Copy link
Contributor

ldez commented Jul 3, 2024

I understand what you called "noise" (the useless patch version), I had the same reaction at the beginning.

I guess since 1.22 correct?

As I didn't find a reference, I will not be able to really answer this question.
From my memories, yes and no, as go1.21 introduced the behavior.

But without an official reference, I will not fight against this change.

@crazy-max
Copy link
Contributor Author

crazy-max commented Jul 3, 2024

Is there an error youre observing?

Now I'm forced to set 1.21.0 as version in go.mod when I vendor: https://github.com/moby/buildkit/actions/runs/9779428021/job/26998669203?pr=5118#step:5:539

Nothing functional that's just a nit but still...

I guess I will have the same issue in the future if someone is doing the same so not directly directed to you but go toolchain shenanigans 😅

@ldez
Copy link
Contributor

ldez commented Jul 3, 2024

I guess I will have the same issue in the future if someone is doing the same so not directly directed to you but go toolchain shenanigans 😅

I don't take it badly: I did it because I was thinking it was a good practice.

I'm annoyed not to find the reference.

@ldez
Copy link
Contributor

ldez commented Jul 3, 2024

I think I have a kind of reference:

Go 1.21 introduces a small change to the numbering of releases. In the past, we used Go 1.N to refer to both the overall Go language version and release family as well as the first release in that family.
https://tip.golang.org/doc/go1.21

This means that go1.21 is now a "release family" and go1.21.0 is a version.

At go 1.21 or higher:

  • The go line declares a required minimum version of Go to use with this module.
  • ...

https://go.dev/ref/mod#go-mod-file-go

The go directive should theoretically declare a version (the minimum version).

I know it's not really explicit, and I'm sure I read an explicit message about that, I'm annoyed to not find it.

@crazy-max
Copy link
Contributor Author

I think I have a kind of reference:

Go 1.21 introduces a small change to the numbering of releases. In the past, we used Go 1.N to refer to both the overall Go language version and release family as well as the first release in that family.
https://tip.golang.org/doc/go1.21

This means that go1.21 is now a "release family" and go1.21.0 is a version.

At go 1.21 or higher:

  • The go line declares a required minimum version of Go to use with this module.
  • ...

https://go.dev/ref/mod#go-mod-file-go

The go directive should theoretically declare a version (the minimum version).

I know it's not really explicit, and I'm sure I read an explicit message about that, I'm annoyed to not find it.

Ah thanks for this feedback, ok I get it better now.

@ldez
Copy link
Contributor

ldez commented Jul 3, 2024

The PR moby/buildkit#5118 has been merged, is your PR still required?

@crazy-max
Copy link
Contributor Author

Not required, I guess we can't do anything about it if we have other dep doing the same and related to your previous comment I think this is fine now. Thanks for your time!

@crazy-max crazy-max closed this Jul 3, 2024
@crazy-max crazy-max deleted the fix-gomod branch July 3, 2024 16:49
@dylan-bourque
Copy link
Member

@ldez I can add a bit of context on the why here because it's been a thorn for us (at work) for a while.

The new automatic toolchain upgrades introduced in Go 1.21 mean that the go directive in each of your dependencies' go.mod files gets fed into an MVS-style selection algorithm, which will update your go.mod file if necessary. It also means that, by default at least, go build, go test, etc. will check that directive and automatically (and silently) download and use that specific version of Go.

As a library, though, the practical effect of this is that updating the Go version in our go.mod will force all downstream consumers to also use that version of Go (or a higher one). I don't think that's a decision that library authors should be making for their consumers, so our go.mod should have the lowest version of Go that includes the features actually in use by the library's code and NOT whatever version any of us happen to be using locally for development.

@cameracker
Copy link

our go.mod should have the lowest version of Go that includes the features actually in use by the library's code and NOT whatever version any of us happen to be using locally for development

Agree. How would we feel about using 1.15 since that was the latest version that the travisci build supported?

@dylan-bourque
Copy link
Member

I think 1.19, or maybe 1.20, would be good since that shouldn't trigger any inadvertent/unwanted Go version changes downstream.

@ldez
Copy link
Contributor

ldez commented Jul 3, 2024

I talk about that inside Gofrs Slack channel.

The new MVS clarification of the meaning of the go directive creates a hard requirement since go1.21.
As a library, like you said, the version of go or dependencies should be handled very carefully.

I updated the Go version to the minimally supported Go version by the Go team, I did it with the knowledge of the impact, and I think that is not a problem to require a Go version supported by the Go team.

The problem exposed through the PR is the difference between "release family" and "version".

@dylan-bourque
Copy link
Member

I was thinking that setting it to 1.20 would be "safe" since everyone should be using at least 1.21 now, but that's definitely not a point I'm going to argue over.as long as we're not pushing consumers onto newer versions of Go.

@dylan-bourque
Copy link
Member

Going forward, though, we shouldn't change the version unless we are using newer language features

@ldez
Copy link
Contributor

ldez commented Jul 3, 2024

I think 1.19, or maybe 1.20, would be good since that shouldn't trigger any inadvertent/unwanted Go version changes downstream.

I don't share this opinion because using unmaintained go1.19 or go1.20 is a security issue.
I think "forcing" the usage of a Go team-supported version (at least to the .0 patch) is not a bad practice.

But I'm against, for a lib, using the latest Go version (currently go1.22), or the latest patch version, unless it's really needed. (I think we agree on that)

I will not update the go directive after each new minor/major release of Go, but if I need to update Go I will update to the "old stable" version (currently go1.21).
(I think we also agree on that)

@thaJeztah
Copy link

The whole versioning has become so complex; fun fact: go now can downgrade language versions per package / file; despite the go1.0 compatibility promise, it may disable features.

Another fun example; backward compatibility code (i.e. files with stubs for old go versions) may no longer always work; see opencontainers/runc#4277 (comment)

@dylan-bourque
Copy link
Member

using unmaintained go1.19 or go1.20

I guess my point is that the version of Go being used won't be what's in this library's go.mod, it will be whatever is specified by the consuming application.

FWIW, I've personally set GOTOOLCHAIN=local on my development machines so that it will always use what I have installed (1.22.5 right now).

@cameracker
Copy link

cameracker commented Jul 3, 2024

Philosophically, I share @dylan-bourque's opinion. I'd prefer that we avoid compelling users to upgrade go versions unless we are leveraging a specific feature of the go version in our mod file. I definitely get that people should be upgrading, but the reality is there are some cases where they don't or can't for a number of reasons and using our package shouldn't get in the way of that. Compelling people to upgrade (as we see in this very PR ;)) does have a side effect of increasing friction to using our package and we want to make the use of our package a smooth experience.

That doesn't mean that we need to support old go versions. We can continue testing against stable and old stable, and guarantee that we fix bugs for latest go and the latest version of our package. I think though that we should not be the pressure that forces people to upgrade.

@thaJeztah
Copy link

Definitely 👍 👍👍👍 on GOTOOLCHAIN=local ... I'm not a big fan of --help meaning; "download a binary and install" 🙈🙈 golang/go#60956

@ldez
Copy link
Contributor

ldez commented Jul 3, 2024

I think we all agree on the background subject: don't update the Go version unless you needed.

I just have no problem defining the old stable as a requirement (not day one, and not the latest patch).

@dylan-bourque
Copy link
Member

not a big fan of --help meaning; "download a binary and install"

@thaJeztah we had someone run into that with go list the other day

@ldez ldez added the wontfix label Jul 3, 2024
@ldez
Copy link
Contributor

ldez commented Jul 5, 2024

I found the reference I was thinking of (yes I never give up 😄)

Code QL, when using at least go1.21, reports a warning:

Screenshot 2024-07-05 at 16-03-09 Code scanning tool status · golangci_golangci-lint

This report is about this file.

module github.com/golangci/golangci-lint/scripts/gen_github_action_config

go 1.21

require (

The documentation referenced inside the screenshot is https://go.dev/doc/toolchain#version

If you add my previous comment with this warning and this doc, we have the full overview of the topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants