Add build tags support #796
Conversation
rgrandl
force-pushed
the
add_build_tags
branch
2 times, most recently
from
a7cd571
to
874b8bf
Compare
`go build` allows the user to specify build tags. These tags enable the user to discard files based on various tags when they build the application binary. `weaver generate` which is a wrapper around `go build` doesn't allow the user to specify build tags. This PR adds built tags support for the `weaver generate` command. The syntax of passing build tags to the `weaver generate` command is similar to how build tags are specified when using `go build`. E.g., weaver generate -tags good,prod weaver generate --tags=good
rgrandl
force-pushed
the
add_build_tags
branch
from
874b8bf
to
f3711d8
Compare
cmd/weaver/main.go
Outdated
| @@ -74,11 +74,25 @@ func main() { | |||
| switch flag.Arg(0) { | |||
| case "generate": | |||
| generateFlags := flag.NewFlagSet("generate", flag.ExitOnError) | |||
| tags := generateFlags.String("tags", "not specified", "Optional tags for the generate command") | |||
There was a problem hiding this comment.
I am not quite sure why we need "not specified". Can't we just say that if empty, we add no tags except for our internal weaver tag, and if non-empty, we add the specified tags + our internal weaver tag.
If we really need to detect whether or not tags were specified on the command line, a less messy alternative to "not specified" would be to call flags.Visit() to see if it reports "tags" as one of the flags set on the command line.
There was a problem hiding this comment.
Good point. I think I implemented this before understanding that I need to define tags as a flag on generateFlags instead of defining a new flag set.
cmd/weaver/main.go
Outdated
| if err := generate.Generate(".", flag.Args()[1:], generate.Options{}); err != nil { | ||
| buildTags := "--tags=ignoreWeaverGen" | ||
| if *tags != "not specified" { // tags flag was specified | ||
| if *tags == "" || *tags == "." { |
There was a problem hiding this comment.
Why do we need a special case for "."?
There was a problem hiding this comment.
We don't. Removed all this complicated logic.
cmd/weaver/main.go
Outdated
| // extra validation at some point. | ||
| buildTags = buildTags + "," + *tags | ||
| } | ||
| idx := len(flag.Args()) - len(generateFlags.Args()) |
There was a problem hiding this comment.
Why do we need to use flag.Args()[idx:] instead passing generateFlags.Args() to generate.Generate?
internal/tool/generate/generator.go
Outdated
| @@ -54,7 +54,7 @@ const ( | |||
| Usage = `Generate code for a Service Weaver application. | |||
|
|
|||
| Usage: | |||
| weaver generate [packages] | |||
| weaver generate [tags] [packages] | |||
There was a problem hiding this comment.
Change to:
weaver generate [-tags taglist] [packages]
| // If non-nil, use the specified function to report warnings. | ||
| Warn func(error) | ||
| Warn func(error) // If non-nil, use the specified function to report warnings | ||
| BuildTags string |
There was a problem hiding this comment.
It seems like the BuildTags fields includes the "-tags=" prefix. That seems a bit wrong to me. Could just contain the tags, and we would add the -tags= prefix below when setting BuildFlags:
cf.BuildFlags = []string{"-tags", opt.BuildTags}
cmd/weaver/main.go
Outdated
| generateFlags.Usage = func() { | ||
| fmt.Fprintln(os.Stderr, generate.Usage) | ||
| } | ||
| generateFlags.Parse(flag.Args()[1:]) | ||
| if err := generate.Generate(".", flag.Args()[1:], generate.Options{}); err != nil { | ||
| buildTags := "--tags=ignoreWeaverGen" |
There was a problem hiding this comment.
See below. I think we should just have "ignoreWeaverGen" here and supply --tags= prefix later.
| @@ -237,6 +238,80 @@ func TestGenerator(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| // TestGeneratorBuildWithTags runs "weaver generate" on all the files in | |||
| // testdata/tags and checks if the command succeeds. Each file should have some build tags. | |||
| func TestGeneratorBuildWithTags(t *testing.T) { | |||
There was a problem hiding this comment.
Why can't we use runGenerator for this test?
There was a problem hiding this comment.
runGenerator copies every single file in a temp directory, and tries to build it. I chose this approach because I wanted to run go build on all the files in the directory and check that weaver_gen contains things only for the good files. That adds a little bit of boilerplate code for sure. If you have a strong preference, I can switch to calling runGenerator instead.
There was a problem hiding this comment.
I am fine with you keeping what you have, but I don't quite understand the downside of using runGenerator. It would copy all of the files and build it. You can still check that weaver_gen.go does not contain the components defined in the bad file.
| @@ -219,7 +225,7 @@ func TestGenerator(t *testing.T) { | |||
| } | |||
|
|
|||
| // Run "weaver generate". | |||
| output, err := runGenerator(t, dir, filename, contents, []string{"sub1", "sub2"}) | |||
| output, err := runGenerator(t, dir, filename, contents, []string{"sub1", "sub2"}, []string{}) | |||
There was a problem hiding this comment.
Can you just pass nil for the empty slice?
| gobuild := exec.Command("go", "build") | ||
|
|
||
| var buildTagsArg string | ||
| if len(buildTags) > 0 { |
There was a problem hiding this comment.
I thought the earlier code always added "ignoreWeaveGen". So can we supply -tags unconditionally?
There was a problem hiding this comment.
Great observation. Fixed
go buildallows the user to specify build tags. These tags enable the user to discard files based on various tags when they build the application binary.weaver generatewhich is a wrapper aroundgo builddoesn't allow the user to specify build tags.This PR adds built tags support for the
weaver generatecommand.The syntax of passing build tags to the
weaver generatecommand is similar to how build tags are specified when usinggo build.E.g.,
weaver generate -tags good,prod
weaver generate --tags=good