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/go: continue conversion to bug-resistant //go:build constraints #41184

Closed
rsc opened this issue Sep 2, 2020 · 43 comments
Closed

cmd/go: continue conversion to bug-resistant //go:build constraints #41184

rsc opened this issue Sep 2, 2020 · 43 comments

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Sep 2, 2020

In June I posted a draft design for moving from // +build lines with ad-hoc syntax to //go:build lines with standard Boolean expressions; that doc also has links to a video overview and a prototype implementation.

I propose that we adopt this design, with N = 17 in the Transition section, meaning that the prep work would happen in Go 1.16, the main change would land in Go 1.17, and the transition would be finalized in Go 1.18.

@rsc rsc added this to the Proposal milestone Sep 2, 2020
@rsc rsc added this to Active in Proposals Sep 2, 2020
@rsc rsc changed the title proposal: start conversion to bug-resistant //go:build constraints proposal: cmd/go: start conversion to bug-resistant //go:build constraints Sep 2, 2020
@rsc
Copy link
Contributor Author

@rsc rsc commented Sep 16, 2020

Based on the emoji above and the lack of any objections (along with the overwhelmingly positive Reddit thread), this seems like a likely accept.

Loading

@cristaloleg
Copy link

@cristaloleg cristaloleg commented Sep 16, 2020

Loading

@rsc rsc moved this from Active to Likely Accept in Proposals Sep 16, 2020
@rsc
Copy link
Contributor Author

@rsc rsc commented Sep 23, 2020

No change in consensus, so accepted.

Loading

@rsc rsc moved this from Likely Accept to Accepted in Proposals Sep 23, 2020
@rsc
Copy link
Contributor Author

@rsc rsc commented Sep 23, 2020

Please note that accepting this means that //go:build lines will start working in Go 1.17, not Go 1.16.
Don't everyone rush to put them in.

Loading

@rsc rsc removed this from the Proposal milestone Sep 23, 2020
@rsc rsc added this to the Go1.16 milestone Sep 23, 2020
@rsc rsc changed the title proposal: cmd/go: start conversion to bug-resistant //go:build constraints cmd/go: start conversion to bug-resistant //go:build constraints Sep 23, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 12, 2020

Change https://golang.org/cl/240600 mentions this issue: go/build: reject //go:build without // +build

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 12, 2020

Change https://golang.org/cl/240553 mentions this issue: cmd/go: add IgnoredOtherFiles to go list; pass IgnoredFiles to vet

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 12, 2020

Change https://golang.org/cl/240601 mentions this issue: cmd/compile: reject misplaced go:build comments

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 12, 2020

Change https://golang.org/cl/240602 mentions this issue: cmd/asm: reject misplaced go:build comments

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 12, 2020

Change https://golang.org/cl/240554 mentions this issue: cmd/vet: look at ignored files in buildtag check

Loading

gopherbot pushed a commit that referenced this issue Oct 12, 2020
Show constraint-ignored non-.go files in go list, as Package.IgnoredOtherFiles
(same as go/build's IgnoredOtherFiles).

Pass full list of ignored files to vet, to help buildtag checker.

For #41184.

Change-Id: I749868de9082cbbc1efbc59370783c8c82fe735f
Reviewed-on: https://go-review.googlesource.com/c/go/+/240553
Trust: Russ Cox <rsc@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
gopherbot pushed a commit that referenced this issue Oct 13, 2020
We are converting from using error-prone ad-hoc syntax // +build lines
to less error-prone, standard boolean syntax //go:build lines.
The timeline is:

Go 1.16: prepare for transition
 - Builds still use // +build for file selection.
 - Source files may not contain //go:build without // +build.
 - Builds fail when a source file contains //go:build lines without // +build lines. <<<

Go 1.17: start transition
 - Builds prefer //go:build for file selection, falling back to // +build
   for files containing only // +build.
 - Source files may contain //go:build without // +build (but they won't build with Go 1.16).
 - Gofmt moves //go:build and // +build lines to proper file locations.
 - Gofmt introduces //go:build lines into files with only // +build lines.
 - Go vet rejects files with mismatched //go:build and // +build lines.

Go 1.18: complete transition
 - Go fix removes // +build lines, leaving behind equivalent // +build lines.

This CL provides part of the <<< marked line above in the Go 1.16 step:
rejecting files containing //go:build but not // +build.
The standard go command checks only consider the top of the file.
This compiler check, along with a separate go vet check for ignored files,
handles the remainder of the file.

For #41184.

Change-Id: I014006eebfc84ab5943de18bc90449e534f150a2
Reviewed-on: https://go-review.googlesource.com/c/go/+/240601
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Oct 13, 2020
We are converting from using error-prone ad-hoc syntax // +build lines
to less error-prone, standard boolean syntax //go:build lines.
The timeline is:

Go 1.16: prepare for transition
 - Builds still use // +build for file selection.
 - Source files may not contain //go:build without // +build.
 - Builds fail when a source file contains //go:build lines without // +build lines. <<<

Go 1.17: start transition
 - Builds prefer //go:build for file selection, falling back to // +build
   for files containing only // +build.
 - Source files may contain //go:build without // +build (but they won't build with Go 1.16).
 - Gofmt moves //go:build and // +build lines to proper file locations.
 - Gofmt introduces //go:build lines into files with only // +build lines.
 - Go vet rejects files with mismatched //go:build and // +build lines.

Go 1.18: complete transition
 - Go fix removes // +build lines, leaving behind equivalent // +build lines.

This CL provides part of the <<< marked line above in the Go 1.16 step:
rejecting files containing //go:build but not // +build.

Reject any //go:build comments found after actual assembler code
(include #include etc directives), because the go command itself
doesn't read that far.

For #41184.

Change-Id: Ib460bfd380cce4239993980dd208afd07deff3f1
Reviewed-on: https://go-review.googlesource.com/c/go/+/240602
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Oct 13, 2020
We are converting from using error-prone ad-hoc syntax // +build lines
to less error-prone, standard boolean syntax //go:build lines.
The timeline is:

Go 1.16: prepare for transition
 - Builds still use // +build for file selection.
 - Source files may not contain //go:build without // +build.
 - Builds fail when a source file contains //go:build lines without // +build lines. <<<

Go 1.17: start transition
 - Builds prefer //go:build for file selection, falling back to // +build
   for files containing only // +build.
 - Source files may contain //go:build without // +build (but they won't build with Go 1.16).
 - Gofmt moves //go:build and // +build lines to proper file locations.
 - Gofmt introduces //go:build lines into files with only // +build lines.
 - Go vet rejects files with mismatched //go:build and // +build lines.

Go 1.18: complete transition
 - Go fix removes // +build lines, leaving behind equivalent // +build lines.

This CL provides part of the <<< marked line above in the Go 1.16 step:
rejecting files containing //go:build but not // +build.

For #41184.

Change-Id: I29b8a789ab1526ab5057f613d5533bd2060ba9cd
Reviewed-on: https://go-review.googlesource.com/c/go/+/240600
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 13, 2020

Change https://golang.org/cl/261958 mentions this issue: cmd: go get golang.org/x/tools@d88ec18 && go mod vendor

Loading

gopherbot pushed a commit that referenced this issue Aug 13, 2021
CL 294430 made packages in std and cmd modules use Go 1.17 gofmt format,
adding //go:build lines. This change applies the same formatting to some
more packages that 'go fmt' missed (e.g., syscall/js, runtime/msan), and
everything else that is easy and safe to modify in bulk.

Consider the top-level test directory, testdata, and vendor directories
out of scope, since there are many files that don't follow strict gofmt
formatting, often for intentional and legitimate reasons (testing gofmt
itself, invalid Go programs that shouldn't crash the compiler, etc.).

That makes it easy and safe to gofmt -w the .go files that are found
with gofmt -l with aforementioned directories filtered out:

	$ gofmt -l . 2>/dev/null | \
		grep -v '^test/' | \
		grep -v '/testdata/' | \
		grep -v '/vendor/' | wc -l
	      51

None of the 51 files are generated. After this change, the same command
prints 0.

For #41184.

Change-Id: Ia96ee2a0f998d6a167d4473bcad17ad09bc1d86e
Reviewed-on: https://go-review.googlesource.com/c/go/+/341009
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Trust: Dmitri Shuralyov <dmitshur@golang.org>
@toothrot
Copy link
Contributor

@toothrot toothrot commented Aug 25, 2021

Reminder that this is an early-in-cycle release-blocker for Go 1.18 and should be addressed soon.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 25, 2021

Change https://golang.org/cl/344955 mentions this issue: all: go fix -fix=buildtag std (except for bootstrap deps)

Loading

@heschi
Copy link
Contributor

@heschi heschi commented Sep 29, 2021

ping @rsc -- to me this seems like something we would like to get in well before the freeze?

Loading

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Oct 8, 2021

For quick reference, the outstanding CLs for this issue that I'm aware of and their status as of this posting:

  • CL 240611 - includes "For #​41184", has +2, outstanding TryBot failures, one trivial code review comment, needs rebasing
  • CL 344955 - includes "Fixes #​41184", unreviewed, outstanding TryBot failures, needs rebasing

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 27, 2021

Change https://golang.org/cl/359314 mentions this issue: cmd/dist: implement //go:build parsing

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 28, 2021

Change https://golang.org/cl/359315 mentions this issue: go/build: update for //go:build lines

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 28, 2021

Change https://golang.org/cl/359355 mentions this issue: cmd/go: update for //go:build lines

Loading

gopherbot pushed a commit that referenced this issue Oct 28, 2021
The bootstrap directories are built with the Go 1.4 go command,
and they will retain the // +build lines until we bump the bootstrap
toolchain to Go 1.17 or later.

cmd/dist builds cmd/go and all its dependencies, using the
assembler, compiler, and linker that were built using Go 1.4.
We don't want to have to keep // +build lines in cmd/go and
all its dependencies, so this CL changes cmd/dist to understand
the //go:build lines.

cmd/dist is a standalone Go program that must itself build with
very old Go releases, so we cannot assume go/build/constraint
is available. Instead, implement a trivial parser/evaluator.

For #41184.

Change-Id: I84e259dec3bd3daec3f82024eb3500120f53096d
Reviewed-on: https://go-review.googlesource.com/c/go/+/359314
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Oct 28, 2021
Look for //go:build ignore, not // +build ignore, in deps_test.go.

For #41184.

Change-Id: Iba8617230aa620223e2bc170f18d0c54557318c4
Reviewed-on: https://go-review.googlesource.com/c/go/+/359315
Trust: Russ Cox <rsc@golang.org>
Trust: Josh Bleecher Snyder <josharian@gmail.com>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
gopherbot pushed a commit that referenced this issue Oct 28, 2021
Now that Go 1.17 is out and Go 1.15 is unsupported,
removing // +build lines can be done safely: in the worst case,
if code is compiled using Go 1.16 the toolchain will detect
the presence of a //go:build without // +build and fail the build.
(It will not silently choose the wrong files.)

Note that +build lines will continue to work in Go sources forever.
This just provides a mechanism for users who are done with
Go 1.16 to remove them easily, by running "go fix".

Also update for new generics AST.

For #41184.
Fixes #48978.

Change-Id: I11a432c319e5abd05ad68dda9ccd7a7fdcc8bbb8
Reviewed-on: https://go-review.googlesource.com/c/go/+/240611
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Oct 28, 2021
cmd/go has its own //go:build evaluator, which is needed for
patterns like 'all'. The code is a modified copy of some unexported
routines from the go/build package. Update it by copying those
again and re-modifying them. The modifications are primarily the new
func eval and also ignoring errors.

This CL will need to be backported to Go 1.17, or else Go 1.17
will break when faced with certain //go:build-only repos during
'go list all' or 'go mod tidy'.

For #41184.
Fixes #49198.

Change-Id: Ie0fe3caa8d49004935ecd76d7977f767fe50e317
Reviewed-on: https://go-review.googlesource.com/c/go/+/359355
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Oct 28, 2021
When these packages are released as part of Go 1.18,
Go 1.16 will no longer be supported, so we can remove
the +build tags in these files.

Ran go fix -fix=buildtag std cmd and then reverted the bootstrapDirs
as defined in src/cmd/dist/buildtool.go, which need to continue
to build with Go 1.4 for now.

Also reverted src/vendor and src/cmd/vendor, which will need
to be updated in their own repos first.

Manual changes in runtime/pprof/mprof_test.go to adjust line numbers.

For #41184.

Change-Id: Ic0f93f7091295b6abc76ed5cd6e6746e1280861e
Reviewed-on: https://go-review.googlesource.com/c/go/+/344955
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 28, 2021

Change https://golang.org/cl/359476 mentions this issue: all: manual fixups for //go:build vs // +build

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 28, 2021

Change https://golang.org/cl/359404 mentions this issue: cmd/go: update for //go:build lines

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 28, 2021

Change https://golang.org/cl/359484 mentions this issue: net/http: restore generated // +build comment

Loading

gopherbot pushed a commit that referenced this issue Oct 28, 2021
The upstream cmd/bundle tool does not yet omit it,
and the longtest builders test that the generated
file is not modified.

For #41184.

Change-Id: Ib68f532139ed1436cbaf3a756f300fe60f520cab
Reviewed-on: https://go-review.googlesource.com/c/go/+/359484
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Oct 28, 2021
cmd/go has its own //go:build evaluator, which is needed for
patterns like 'all'. The code is a modified copy of some unexported
routines from the go/build package. Update it by copying those
again and re-modifying them. The modifications are primarily the new
func eval and also ignoring errors.

This CL will need to be backported to Go 1.17, or else Go 1.17
will break when faced with certain //go:build-only repos during
'go list all' or 'go mod tidy'.

For #41184.
Fixes #49198.

Change-Id: Ie0fe3caa8d49004935ecd76d7977f767fe50e317
Reviewed-on: https://go-review.googlesource.com/c/go/+/359355
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/359404
@gopherbot gopherbot closed this in af05d8b Oct 28, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 5, 2021

Change https://golang.org/cl/361480 mentions this issue: all: remove more leftover // +build lines

Loading

gopherbot pushed a commit that referenced this issue Nov 6, 2021
CL 344955 and CL 359476 removed almost all // +build lines, but leaving
some assembly files and generating scripts. Also, some files were added
with // +build lines after CL 359476 was merged. Remove these or rename
files where more appropriate.

For #41184

Change-Id: I7eb85a498ed9788b42a636e775f261d755504ffa
Reviewed-on: https://go-review.googlesource.com/c/go/+/361480
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 9, 2021

Change https://golang.org/cl/362577 mentions this issue: all: add missing //go:build comments

Loading

gopherbot pushed a commit to golang/sys that referenced this issue Nov 10, 2021
These were apparently overlooked in CL 357329, CL 294490, CL 296889,
and other various updates to this module. (I noticed them via gopls
while investigating golang/go#49466.)

Updates golang/go#41184

Change-Id: Id042bb6fe5282e6d528e9315acf2ad29d0df58ba
Reviewed-on: https://go-review.googlesource.com/c/sys/+/362577
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 24, 2021

Change https://golang.org/cl/366894 mentions this issue: cmd/dist: add buildtag parsing test

Loading

gopherbot pushed a commit that referenced this issue Nov 25, 2021
Forgot to 'git add' this test written as part of CL 359314.

For #41184.

Change-Id: I2ebd48fd62a2053c8b16e5a8c48c1e11d1b86d5b
Reviewed-on: https://go-review.googlesource.com/c/go/+/366894
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants