buildtags: support //go:build constraints

[mmcloughlin]

New go:build constraints will be preferred in Go 1.17. Therefore avo must support them.

https://golang.org/design/draft-gobuild
golang/go#25348
golang/go#41184

[FiloSottile]

👋 hi! Do you think you'll have time for this before the end of the month? If you'd like, I can look into contributing the change!

[mmcloughlin]

Hey! Sorry, yes I'll look at it this weekend :)

[dmitshur]

FWIW, in Go 1.17 the go/format package (like the gofmt binary) takes care of this¹, so if used, the output is guaranteed to be gofmt'ed (and continue to be over time if there are changes to gofmt behavior in future versions).

Edit: Hmm, I realize now that go/format can't help with the generated .s files, only .go ones. So it's a complete solution will still be more involved.

¹ https://tip.golang.org/doc/go1.17#go/format

[josharian]

@dmitshur I’m not sure whether go/format will continue to rewrite //+build to //go:build forever. Anyway, the nicer //go:build format will likely end up simplifying this code anyway once it can assume 1.17.

[josharian]

@dmitshur should go/format touch .s files, if only the build header lines? Honestly, I wouldn’t mind if it also normalized the rest of the files either. Worth filing an issue (or two?)?

[dmitshur]

should go/format touch .s files, if only the build header lines?

My personal preference would be to keep the scope of cmd/gofmt (and go/format, which is just an importable Go package version thereof) to operate on .go files only. That's hard enough as it is. :)

[mmcloughlin]

Sorry for the really slow response on this, I've been struggling to keep up with my open-source work these days. Thank you for your input and for working around this issue in the meantime.

There seem to be two issues here, firstly how to fix the immediate problem of avo not producing gofmt'ed output, and secondly how avo should support go:build constraints going forward. The main question is how to reconcile avo's existing buildtags package with the new go/build/constraint package. My proposal is below, any thoughts much appreciated.

Now: Update the printing of build constraints to go via go/format, both for the assembly and stub files printers. Since the formatter does not support assembly, this will need to use a slight hack: construct a source file in memory that just has the +build lines and a dummy package header, run it through format.Source and then extract the comment lines from it. This should be enough to ensure that for now avo output is gofmt-compatible in all Go versions.

Note the code to be updated is:

avo/printer/goasm.go

Lines 46 to 49 in b935256

	if len(f.Constraints) > 0 {
	p.NL()
	p.Printf(f.Constraints.GoString())
	}

avo/printer/stubs.go

Lines 21 to 24 in b935256

	if len(f.Constraints) > 0 {
	s.NL()
	s.Printf(f.Constraints.GoString())
	}

The constraints formatter would just be something like: https://play.golang.org/p/UlV0ZypCbCL.

Go 1.18+: The question here is what to do with the buildtags package and associated APIs:

avo/build/global.go

Lines 62 to 72 in b935256

	// Constraints sets build constraints for the file.
	func Constraints(t buildtags.ConstraintsConvertable) { ctx.Constraints(t) }
	// Constraint appends a constraint to the file's build constraints.
	func Constraint(t buildtags.ConstraintConvertable) { ctx.Constraint(t) }
	// ConstraintExpr appends a constraint to the file's build constraints. The
	// constraint to add is parsed from the given expression. The expression should
	// look the same as the content following "// +build " in regular build
	// constraint comments.
	func ConstraintExpr(expr string) { ctx.ConstraintExpr(expr) }

In practice almost everyone has used the ConstraintExpr() function. The buildtags package provides manipulation of +build constraints much like go/build/constraint, though it's a bit too over-engineered. If avo were written today I would never have written the buildtags package at all.

Therefore I think the best approach is that when Go 1.18 lands avo will migrate to the go/build/constraint package instead.

• Add GoBuild(expr string) function for specifying a go:build style constraint
• Add ir.File.GoBuild field
• Deprecate the buildtags package
• Deprecate the associated build.Constraint*() functions
• Deprecate/remove the ir.File.Constraint field

The alternative is to attempt to maintain the buildtags package and types:

• ConstraintExpr(string) attempts to parse go:build first, fallback to +build. Does this work? At the moment the +build prefix is not required, so are there valid +build expressions that would parse as valid go:build expressions with a different meaning? You could avoid this by having the go:build prefix required, even if the +build was not previously.
• Implement conversion between constraint.Expr and corresponding buildtags package types
• The printer package prints according to Go version

Thanks again!

[mmcloughlin]

Yesterday I landed #208 #209 and #197 which ensures avo does the right thing for Go 1.17, essentially by running the build tags through go/format.

[mmcloughlin]

Okay landed #211 so it should do the right thing for Go 1.18.

So, the current behavior is that avo will output the build constraint syntax consistent with the Go version avo was compiled with. I'm assuming this is going to be what essentially everyone wants, but if anyone needs this to be configurable I could consider it.

This should be enough to unblock people, but I'm going to keep this open because avo should also be extended to support go:build syntax and the go/build/constraint package.

[WojciechMula]

So, the current behavior is that avo will output the build constraint syntax consistent with the Go version avo was compiled with. I'm assuming this is going to be what essentially everyone wants, but if anyone needs this to be configurable I could consider it.

It would be great if we can force old tag creation. We already hit that problem in compress library: https://github.com/klauspost/compress/runs/6327078728?check_suite_focus=true

For me, it would be great if we have setters like EnablePlusBuildSyntax(bool) & EnableGoBuildSyntax(bool).

[mmcloughlin]

We already hit that problem in compress library: https://github.com/klauspost/compress/runs/6327078728?check_suite_focus=true

Yes, I set the code generation jobs in compress to use Go 1.17.x for this reason. klauspost/compress#570 (comment)

I know it's not ideal, but how annoying is it to use Go 1.17 for your code generation?

[WojciechMula]

I know it's not ideal, but how annoying is it to use Go 1.17 for your code generation?

TBH, this would not be convenient. But I realized I can tweak an avo installation locally:

chmod -R +w ~/go/pkg/mod/github.com/mmcloughlin/avo@v0.4.0/buildtags
sed -i -e 's/false/true/' ~/go/pkg/mod/github.com/mmcloughlin/avo@v0.4.0/buildtags/syntax_go118.go

Maybe that's not the nicest approach, but works perfectly on a local machine.

[mmcloughlin]

I'm considering adding a -go flag similar to staticcheck, allowing you to specify the target Go version. By default, this would take the version from the package module (if the Package command was used), or it would fallback to the version of Go avo was built with (current behavior). The build tag syntax could be deduced from the target Go version.

Would this work for you @WojciechMula? Or do you want explicit control over the build tag syntax independent of specifying a target Go version?

[WojciechMula]

@mmcloughlin That would be perfect. However, I'm thinking now if it's really needed. So far I'm the only person who asked about that, and there's a workaround. Additionally, Go 1.16 is already not supported, EOL was in February this year.

[mmcloughlin]

@WojciechMula Yeah, it's common for library authors to match the Go support policy and only support the last two minor versions. I'm curious why compress wants to support 1.16+, but it's not unreasonable.

I'm considering the -go flag because it might have other applications, such as #84.