avo
avo copied to clipboard
buildtags: support //go:build constraints
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
👋 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!
Hey! Sorry, yes I'll look at it this weekend :)
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
@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.
@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?)?
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. :)
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:
https://github.com/mmcloughlin/avo/blob/b935256fa53858859d608444d9f1bde3d952022f/printer/goasm.go#L46-L49 https://github.com/mmcloughlin/avo/blob/b935256fa53858859d608444d9f1bde3d952022f/printer/stubs.go#L21-L24
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:
https://github.com/mmcloughlin/avo/blob/b935256fa53858859d608444d9f1bde3d952022f/build/global.go#L62-L72
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 ago: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 parsego: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 validgo:build
expressions with a different meaning? You could avoid this by having thego:build
prefix required, even if the+build
was not previously. - Implement conversion between
constraint.Expr
and correspondingbuildtags
package types - The
printer
package prints according to Go version
Thanks again!
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
.
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.
So, the current behavior is that
avo
will output the build constraint syntax consistent with the Go versionavo
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)
.
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. https://github.com/klauspost/compress/pull/570#discussion_r857958777
I know it's not ideal, but how annoying is it to use Go 1.17 for your code generation?
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/[email protected]/buildtags
sed -i -e 's/false/true/' ~/go/pkg/mod/github.com/mmcloughlin/[email protected]/buildtags/syntax_go118.go
Maybe that's not the nicest approach, but works perfectly on a local machine.
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?
@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.
@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.