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

Jump to bottom

x/website: adjust release notes to say that cmd/fix's buildtag fix only kicks in with go.mod containing "go 1.18" or later #51873

Closed
mvdan opened this issue Mar 22, 2022 · 11 comments
Closed

x/website: adjust release notes to say that cmd/fix's buildtag fix only kicks in with go.mod containing "go 1.18" or later #51873

mvdan opened this issue Mar 22, 2022 · 11 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@mvdan
Copy link
Member

mvdan commented Mar 22, 2022 

go version go1.18 linux/amd64

https://go.dev/doc/go1.18 says:

In Go 1.18, go fix now removes the now-obsolete // +build lines in modules declaring go 1.17 or later in their go.mod files.

And the original issue agrees:

#41184
https://go.googlesource.com/proposal/+/master/design/draft-gobuild.md

I propose that we adopt this design, with N = 17 in the Transition section,

Go 1.(N+1) would complete the transition. In Go 1.(N+1):
A new fix in go fix will remove // +build stanzas, making sure to leave behind equivalent //go:build lines. The removal only happens when go fix is being run in a module with a go 1.N (or later) line, which is taken as an explicit signal that the developer no longer needs compatibility with Go 1.(N−1). The removal is never done in GOPATH mode, which lacks any such explicit signal.

However, the cmd/fix code incorrectly requires go 1.18 or later for the removal of // +build lines to happen:

const buildtagGoVersionCutoff = 1_18

[...]

func buildtag(f *ast.File) bool {
    if goVersion < buildtagGoVersionCutoff {
        return false
    }

I assume this was a mistake in the code. I only noticed because I have a module with build tags and go 1.17 in its go.mod, signaling that I no longer support Go 1.16.x, and yet I was surprised to see that go fix wouldn't clean up the build tags until I pushed go.mod to go 1.18.

I have the rather trivial fix ready. The point of this issue is that, assuming I'm right, this would need to be backported for Go 1.18.x, to make the release notes actually reflect reality.

cc @rsc @ianlancetaylor
@mvdan mvdan self-assigned this Mar 22, 2022
@gopherbot
Copy link

Change https://go.dev/cl/394675 mentions this issue: cmd/fix: apply buildtag fix when go.mod contains "go 1.17"

@mvdan
Copy link
Member Author

mvdan commented Mar 25, 2022

Friendly ping @rsc @ianlancetaylor or @dmitshur for confirmation that we should indeed backport this to 1.18.1 :) It seems pretty clear to me, but I also don't want to add issues to the milestone without any notice.

@seankhliao seankhliao added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 28, 2022
@mvdan
Copy link
Member Author

mvdan commented Mar 29, 2022

Going ahead with marking this for 1.18.1 for visibility. Happy for the backport issue to be removed if my conclusions are wrong.

@gopherbot, please backport to Go 1.18. This seems like a go fix bug which contradicts the release notes.

@gopherbot gopherbot mentioned this issue Mar 29, 2022
Closed
@gopherbot
Copy link

Backport issue(s) opened: #52011 (for 1.18).

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

@ianlancetaylor
Copy link
Contributor

Sorry for not paying attention to this. I think that we should instead just change the documentation. I think that waiting for another release was the right move here, given that people using 1.17 may still want to support older releases even though we don't support them ourselves.

@mvdan
Copy link
Member Author

mvdan commented Mar 30, 2022

Hmm, OK. I think we then need to abandon the CL and close the backport issue, and close this issue with an x/website CL to tweak the 1.18 changelog. Does that sound right?

@ianlancetaylor
Copy link
Contributor

SGTM. Thanks.

@bcmills
Copy link
Contributor

bcmills commented Mar 30, 2022

Yep, that sounds right.

Go 1.17 shouldn't have applied the fix because when 1.17 was released, 1.16 was still supported (and wouldn't build with the fix applied), and we don't want go fix to introduce a 1.16 incompatibility if the rest of the code happens to remain compatible. (Recall that go 1.17 means “you need Go 1.17 for full fidelity”, not “this won't build at all before Go 1.17”.)

And arguably go fix in a go 1.17 module should do the same thing that Go 1.17 would have done, so go 1.18 does seem like the right threshold here.

@mvdan mvdan changed the title cmd/fix: buildtag fix only kicks in with go.mod containing "go 1.18" or later x/website: adjust release notes to say that cmd/fix's buildtag fix only kicks in with go.mod containing "go 1.18" or later Mar 30, 2022
@gopherbot gopherbot added this to the Unreleased milestone Mar 30, 2022
@mvdan mvdan removed their assignment Mar 30, 2022
@mvdan
Copy link
Member Author

mvdan commented Mar 30, 2022

Closed the backport issue and retitled. I'll leave the x/website fix to someone else, as I'm not able to submit changes in that repo.

@ianlancetaylor
Copy link
Contributor

For the record you should be able to send in changes to the repo, although (unfortunately) two other people with appropriate access need to approve.

@gopherbot
Copy link

Change https://go.dev/cl/396879 mentions this issue: _content/doc/go1.18: go fix requires go.mod go 1.18 for go:build change

passionSeven added a commit to passionSeven/website that referenced this issue Oct 18, 2022
@passionSeven
_content/doc/go1.18: go fix requires go.mod go 1.18 for go:build change
7c27ec6 
Fixes golang/go#51873

Change-Id: Ied51c36a30cd18ff4a84d164e62df2690931d94f
Reviewed-on: https://go-review.googlesource.com/c/website/+/396879
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Trust: Ian Lance Taylor <iant@google.com>
@golang golang locked and limited conversation to collaborators Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@ianlancetaylor @mvdan @bcmills @gopherbot @seankhliao