datadog-agent icon indicating copy to clipboard operation
datadog-agent copied to clipboard

[BUG] go.mod version set to specific patch version

Open ChronosMasterOfAllTime opened this issue 1 year ago • 8 comments
trafficstars

go.mod#L3 Has a specific patch version set (1.22.0).

Looks like it was introduced in #29154. Please fix the go.mod file to be 1.22 instead.

Agent Environment N/A

Describe what happened: go.mod version accidentally got set to 1.22.0. This ties your package to a specific patch version of 1.22. For those using a later go version it will inject 1.22.0 with toolchain 1.22.x (where x is the patch version)

Describe what you expected: Should be 1.22

Steps to reproduce the issue: Import latest from github.com/DataDog/datadog-agent/pkg/obfuscate

Additional environment details (Operating System, Cloud provider, etc): MacOS/Linux

ChronosMasterOfAllTime avatar Sep 13 '24 13:09 ChronosMasterOfAllTime

Hi @ChronosMasterOfAllTime, thanks for opening an issue !

This is not a recent change, the modules declared in this repo have been using the bugfix version since https://github.com/DataDog/datadog-agent/pull/22908 (7 months ago).

The reason is that when we import a module then we are forced to use a more recent Go version than the one in the go.mod file, for example we depend on golang.org/x/tools v0.25.0 which uses Go version 1.22.0, so we need to specify at least 1.22.0 ourselves.

What is your use case for needing to import a module from datadog-agent ? (eg. is it due to using opentelemetry ?) Do you only need pkg/obfuscate ?

pgimalac avatar Sep 16 '24 17:09 pgimalac

It's an indirect depency of dd-trace-go.v1.

Unless there's a very specific reason to use go 1.22.0 instead of go 1.22.x (where x is the latest patch revision) you will be ok to changing the mod file to go 1.22 as that doesn't put a hard dependency on having to use go 1.22.0.

ChronosMasterOfAllTime avatar Sep 16 '24 18:09 ChronosMasterOfAllTime

To be clear, you are not forced to use 1.22.0, just a version which is at least 1.22.0, it's the same constraint as setting 1.22

As per https://go.dev/doc/modules/gomod-ref#go-notes:

The go directive sets the minimum version of Go required to use this module

Does that answer your question ?

pgimalac avatar Sep 16 '24 18:09 pgimalac

The problem is it modifies our go mod to include a toolchain line with the patch version. This is the only project that currently causes this to occur in our go mod file.

ChronosMasterOfAllTime avatar Sep 16 '24 21:09 ChronosMasterOfAllTime

Is the Go version in your go.mod older than 1.22.0 ? (note that 1.22 is considered older than 1.22.0) If so you probably just need to set the go directive to 1.22.0 (or anything more recent), and then you don't need a toolchain directive (similarly to what we do)

pgimalac avatar Sep 17 '24 10:09 pgimalac

Is the Go version in your go.mod older than 1.22.0 ? (note that 1.22 is considered older than 1.22.0) If so you probably just need to set the go directive to 1.22.0 (or anything more recent), and then you don't need a toolchain directive (similarly to what we do)

No, my go version is 1.22.7. Your full go mod version declaration is forcing the toolchain to show up because my version is newer. If you set it to go 1.22 in your file, everything will be fine. It means any version of go 1.22 or later is acceptable. This is why I am asking you to fix it.

ChronosMasterOfAllTime avatar Sep 17 '24 12:09 ChronosMasterOfAllTime

I tried to reproduce by creating a module using go 1.22.7 directive, then importing github.com/DataDog/datadog-agent/pkg/obfuscate from that module, and it worked as expected, go mod tidy did not add a toolchain directive.

Can you share a minimal reproducible example of the issue ?

As I mentioned in my first comment the datadog-agent project has some dependencies which use go 1.22.0 directive, so we have to use a bugfix version in the directive too... We could change only pkg/obfuscate (not the other modules) but that makes it more annoying to maintain for us, so if we can avoid that would be best.

pgimalac avatar Sep 17 '24 20:09 pgimalac

I am mistaken on the package. It's coming from datadog-agent: https://github.com/DataDog/datadog-agent/blob/main/go.mod#L3

I am curious why you're deadset against doing a find and replace of go 1.22.0 -> go 1.22?

ChronosMasterOfAllTime avatar Sep 17 '24 21:09 ChronosMasterOfAllTime

Ok then we just definitely can't... As I mentioned, in my first comment:

The reason is that when we import a module then we are forced to use a more recent Go version than the one in the go.mod file, for example we depend on golang.org/x/tools v0.25.0 which uses Go version 1.22.0, so we need to specify at least 1.22.0 ourselves.

Since we have a dependency which uses 1.22.0, we HAVE TO use a more recent Go version, and 1.22 is considered "older" than 1.22.0.

Let me know if something is unclear.

pgimalac avatar Sep 18 '24 12:09 pgimalac

and 1.22 is considered "older" than 1.22.0

specifying go 1.22 vs go 1.22.0 does the following:

You MUST be on ANY version of go greater than or equal to 1.22.0 (this includes future minor version of go - e.g. 1.23.x).

By specifying go 1.22.0 you are effectively saying ONLY 1.22.0 is OK for use.

from chatGPT feeding in the go mod file reference:

In a go.mod file, the go directive is used to indicate the version of the Go language that the module is intended to be used with. Here’s the difference between the two declarations you mentioned:

go 1.22

  • This specifies the major and minor version of Go, which is Go 1.22 in this case.
  • It means the module is intended to work with Go 1.22 and any compatible subsequent patch releases (e.g., 1.22.1, 1.22.2, etc.).
  • This is the preferred and standard way to declare the Go version in a go.mod file. It allows flexibility for patch updates, which typically include bug fixes and minor improvements.

go 1.22.0

  • This specifies the exact version of Go, which is Go 1.22.0 in this case.
  • It is less common to specify the exact patch version in a go.mod file.
  • Using the exact version might be unnecessarily restrictive, as it would not automatically take advantage of any bug fixes or improvements included in subsequent patch releases (e.g., 1.22.1).

Preferred Method

The preferred method is to use go 1.22 rather than go 1.22.0. This allows the module to benefit from any patch updates within the 1.22 series without changing the go.mod file, adhering to the principle of semantic versioning.

Here is an example of how it looks in a go.mod file:

module example.com/my/module

go 1.22

This indicates that the module is compatible with Go 1.22 and any later patch versions, providing a good balance between stability and the ability to receive bug fixes and improvements.

ChronosMasterOfAllTime avatar Sep 18 '24 17:09 ChronosMasterOfAllTime

By specifying go 1.22.0 you are effectively saying ONLY 1.22.0 is OK for use.

No, as per go.dev/doc/modules/gomod-ref#go-notes:

The go directive sets the minimum version of Go required to use this module

This is just a minimum version, not a specific required version.

As explained in https://go.dev/doc/toolchain, by default if your installed Go version is more recent than the one specified in the go directive, then your installed version will be used, it won't downgrade to the older toolchain. Also if you have go 1.22 in your go.mod, and your installed version is older, then running any command will fail as it will try to download go1.22 (which is not a valid go version):

$ go version # run any Go command, `go` is `go1.21.5` here
go: downloading go1.22 (darwin/arm64)
go: download go1.22 for darwin/arm64: toolchain not available

It is less common to specify the exact patch version in a go.mod file. The preferred method is to use go 1.22 rather than go 1.22.0.

I think this was true until go1.21, but now it's the new normal, as summarized in https://github.com/golang/go/issues/62278#issuecomment-1698829945:

Before 1.21 release X.Y.Z value (1.20.1) would be invalid so you couldn't put it there.

With 1.21 release you most likely want to put X.Y.Z there since most user would want to put there a real working version, not a dev release. To add to confusion X.Y value technically works, but it may or may not depending on what was released.

So the way I see it we went from never put .0 to always put .0 (0 being point release number).

I'll close this issue as we can't change the version in our go.mod file to 1.22 even if we wanted to, since some of our dependencies use 1.22.0 themselves. Feel free to re-open if something is unclear.

pgimalac avatar Sep 19 '24 13:09 pgimalac

@pgimalac it's unfortunate you closed this. The purpose of specifying the patch version is if the following is true:

  • The dependent package cannot support ANY patch rendition of a given major/minor release.
    • For example, if the package can only support 1.22.2 and beyond, then you would specify 1.22.2
    • However, specifying 1.22.0 vs 1.22 is superfluous and doesn't provide any value since they're both effectively the same statement.

I wont spend more time on this as I have more important things to attend to. I thought this would have been an easy update not met with so much scrutiny. Thank you.

ChronosMasterOfAllTime avatar Sep 19 '24 15:09 ChronosMasterOfAllTime

From that same toolchain doc:

The syntax ‘1.N’ is called a “language version”. It denotes the overall family of Go releases implementing that version of the Go language and standard library.

AdamDrewsTR avatar Sep 19 '24 16:09 AdamDrewsTR

By specifying go 1.22.0 you are effectively saying ONLY 1.22.0 is OK for use.

No, as per go.dev/doc/modules/gomod-ref#go-notes:

The go directive sets the minimum version of Go required to use this module

This is just a minimum version, not a specific required version.

As explained in https://go.dev/doc/toolchain, by default if your installed Go version is more recent than the one specified in the go directive, then your installed version will be used, it won't downgrade to the older toolchain. Also if you have go 1.22 in your go.mod, and your installed version is older, then running any command will fail as it will try to download go1.22 (which is not a valid go version):

$ go version # run any Go command, `go` is `go1.21.5` here
go: downloading go1.22 (darwin/arm64)
go: download go1.22 for darwin/arm64: toolchain not available

It is less common to specify the exact patch version in a go.mod file. The preferred method is to use go 1.22 rather than go 1.22.0.

I think this was true until go1.21, but now it's the new normal, as summarized in golang/go#62278 (comment):

Before 1.21 release X.Y.Z value (1.20.1) would be invalid so you couldn't put it there. With 1.21 release you most likely want to put X.Y.Z there since most user would want to put there a real working version, not a dev release. To add to confusion X.Y value technically works, but it may or may not depending on what was released. So the way I see it we went from never put .0 to always put .0 (0 being point release number).

I'll close this issue as we can't change the version in our go.mod file to 1.22 even if we wanted to, since some of our dependencies use 1.22.0 themselves. Feel free to re-open if something is unclear.

Are you saying that the 1.22 release candidates cannot be used, and the first release, 1.22.0, or higher must be used?

AdamDrewsTR avatar Sep 19 '24 16:09 AdamDrewsTR

The dependent package cannot support ANY patch rendition of a given major/minor release. For example, if the package can only support 1.22.2 and beyond, then you would specify 1.22.2 However, specifying 1.22.0 vs 1.22 is superfluous and doesn't provide any value since they're both effectively the same statement.

Yes I agree (although the way I see it it means setting 1.22.0 rather than 1.22 is not a big deal), I think the only differences are:

  • using 1.22 will make Go fail if the installed version is older (while it works fine when specifying 1.22.0)
  • as hinted by @AdamDrewsTR if specifying 1.22.0 then you can't use a 1.22 release candidate

But neither really matters in this case.

I thought this would have been an easy update not met with so much scrutiny

Feel free to open a PR and I'll be happy to review it, but as I mentioned several times you will find that it is not possible as we have external dependencies which use go 1.22.0, so we have to do the same.

If you are not convinced you can try creating a new Go module, setting go 1.22 in the go.mod file, and then importing golang.org/x/tools v0.25.0, you will see that when running go mod tidy it will change the go directive to 1.22.0 as it needs to be superior or equal to the one of golang.org/x/tools v0.25.0 which is also 1.22.0.

Are you saying that the 1.22 release candidates cannot be used, and the first release, 1.22.0, or higher must be used?

@AdamDrewsTR kind of yes, golang.org/x/tools v0.25.0 specifies go 1.22.0 so maybe there is a reason it can't be used with an RC version, and in any case it means the datadog-agent project can't be built with a 1.22 release candidate.

If you are able to make golang.org/x/tools to downgrade the go directive to 1.22 then it's possible we can do the same in the datadog-agent project.

pgimalac avatar Sep 19 '24 19:09 pgimalac