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

cmd/compile: Go 1.22 changes support for modules that declare go 1.0 #65528

Closed
dmitshur opened this issue Feb 5, 2024 · 6 comments
Closed

cmd/compile: Go 1.22 changes support for modules that declare go 1.0 #65528

dmitshur opened this issue Feb 5, 2024 · 6 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Feb 5, 2024

Go version

go version go1.22rc2 darwin/arm64

Output of go env in your module/workspace:

(Shouldn't be relevant; happy to provide otherwise.)

What did you do?

I used a Go 1.22-ish toolchain, instead of a 1.21-ish one, to install the command go.chromium.org/luci/cipd/client/cmd/cipd@latest. It failed because one of its dependencies has a go module directive with an unusually-low value of "1.0" here, reporting a build error as:

# github.com/danjacques/gofslock/fslock
compile: invalid value "go1" for -lang: should be something like "go1.12"

Here's a smaller reproducer:

$ cd $(mktemp -d)
$ go mod init test
$ go get [email protected] toolchain@none
$ echo 'package p; import _ "github.com/danjacques/gofslock/fslock"' > p.go
$ go get . github.com/danjacques/[email protected]

What did you see happen?

$ go test
# github.com/danjacques/gofslock/fslock
compile: invalid value "go1" for -lang: should be something like "go1.12"
FAIL	test [build failed]

$ GOTOOLCHAIN=go1.21.6 go test
?   	test	[no test files]
$ echo $?
0

What did you expect to see?

I'm not sure if this edge-case should work or not.

I read over https://go.dev/doc/toolchain#version, which includes:

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.

But doesn't seem to specify what the minimum value of N is.

The new go/version package has IsValid, which reports true for "go1.0" and "go1". (https://go.dev/play/p/82xhtZbE9N9?v=gotip)

@bcmills bcmills changed the title cmd/go: Go 1.22 changes support for very old Go language versions cmd/go,cmd/compile: Go 1.22 changes support for modules that declare go 1.0 Feb 5, 2024
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Feb 5, 2024
@bcmills bcmills added the GoCommand cmd/go label Feb 5, 2024
@bcmills
Copy link
Contributor

bcmills commented Feb 5, 2024

Both gover.Lang and version.Lang have tests that a bare "1" is considered a valid language version:

On the other hand, cmd/compile expects to parse the -lang flag using a regular expression:

So I think two things need to happen here:

  1. We should double check (with @rsc) that version.Lang("go1.0") really is intended to be go1 and not go1.0, and
  2. assuming that it is, cmd/compile should be updated so that the -lang flag accepts all versions for which version.Lang(x) == x.

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 5, 2024
@bcmills bcmills added this to the Go1.23 milestone Feb 5, 2024
@bcmills bcmills added release-blocker and removed GoCommand cmd/go labels Feb 5, 2024
@bcmills bcmills changed the title cmd/go,cmd/compile: Go 1.22 changes support for modules that declare go 1.0 cmd/compile: Go 1.22 changes support for modules that declare go 1.0 Feb 7, 2024
@rsc
Copy link
Contributor

rsc commented Feb 7, 2024

Lang("go1.0") = "go1" is correct; the compiler should accept "go1" in addition what it currently accepts namely "go1.0".

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/562322 mentions this issue: cmd/compile: accept -lang=go1 as -lang=go1.0

@bcmills
Copy link
Contributor

bcmills commented Feb 8, 2024

@gopherbot, please backport to Go 1.22. This was a regression in 1.22.0 and the fix is trivial.

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #65619 (for 1.22).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. FixPending Issues that have a fix which has not yet been reviewed or submitted. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 8, 2024
@mdempsky
Copy link
Contributor

mdempsky commented Feb 8, 2024

Lang("go1.0") = "go1" is correct; the compiler should accept "go1" in addition what it currently accepts namely "go1.0".

On go.dev/cl/562322, I pointed out that cmd/go does not support "go 1" in go.mod files. @bcmills says this is expected because "go 1.0 is not the canonical name for that version."

I'll also point out that go mod edit -go=1 fails. (Both -toolchain=go1 and -toolchain=go1.0 are supported though.)

If cmd/go won't let its users spell Go language version "1.0" as "1", why did it start spelling "-lang=go1.0" as "-lang=go1"? Why is it cmd/compile's responsibility to now accept either spelling? Can cmd/go instead revert to the original spelling that it still requires from users?

Thanks.

@github-project-automation github-project-automation bot moved this from Todo to Done in Go Compiler / Runtime Feb 8, 2024
kxxt added a commit to kxxt/gofslock that referenced this issue Feb 11, 2024
Go 1.22 breaks modules that declare go 1.0: golang/go#65528

This PR modifies the Go version in go.mod to 1.14 because
module must run on Go version 1.14 or later, as documented in https://go.dev/doc/modules/gomod-ref
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
Fixes golang#65528.

Change-Id: I55fef8cf7be4654c7242462d45f12999e0c91c02
Reviewed-on: https://go-review.googlesource.com/c/go/+/562322
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Bryan Mills <[email protected]>
Arno500 added a commit to Arno500/plex-richpresence that referenced this issue Feb 19, 2024
bobg added a commit to bobg/mingo that referenced this issue Mar 11, 2024
bobg added a commit to bobg/mingo that referenced this issue Oct 31, 2024
* Make it possible for test .go files to import packages.

* Fix import parsing and rendering.

* Some more test coverage.

* Refactor to remove a lot of p.isMax calls.

* Add -strict and more test coverage.

* Some refactoring and removing unnecessary lines of code.

* Oops, finish adding -strict. Add test coverage.

* Remove workaround now that a fix for golang/go#65528 is backported to Go 1.22.

* Fix merge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

5 participants