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

Add build tags support #796

Merged
merged 4 commits into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion cmd/weaver/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,19 @@ func main() {
switch flag.Arg(0) {
case "generate":
generateFlags := flag.NewFlagSet("generate", flag.ExitOnError)
tags := generateFlags.String("tags", "", "Optional tags for the generate command")
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 := "ignoreWeaverGen"
if *tags != "" { // tags flag was specified
// TODO(rgrandl): we assume that the user specify the tags properly. I.e.,
// a single tag, or a list of tags separated by comma. We may want to do
// extra validation at some point.
buildTags = buildTags + "," + *tags
}
if err := generate.Generate(".", generateFlags.Args(), generate.Options{BuildTags: buildTags}); err != nil {
fmt.Fprintln(os.Stderr, err)
os.Exit(1)
}
Expand Down
31 changes: 22 additions & 9 deletions internal/tool/generate/generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ const (
Usage = `Generate code for a Service Weaver application.

Usage:
weaver generate [packages]
weaver generate [-tags taglist] [packages]

Description:
"weaver generate" generates code for the Service Weaver applications in the
Expand All @@ -64,6 +64,9 @@ Description:
file in the package's directory. For example, "weaver generate . ./foo" will
create ./weaver_gen.go and ./foo/weaver_gen.go.

You specify build tags for "weaver generate" in the same way you specify build
tags for go build. See "go help build" for more information.

You specify packages for "weaver generate" in the same way you specify
packages for go build, go test, go vet, etc. See "go help packages" for more
information.
Expand All @@ -86,13 +89,21 @@ Examples:
weaver generate ./foo

# Generate code for all packages in all subdirectories of current directory.
weaver generate ./...`
weaver generate ./...

# Generate code for all files that have a "//go:build good" line at the top of
the file.
weaver generate -tags good

# Generate code for all files that have a "//go:build good,prod" line at the
top of the file.
weaver generate -tags good,prod`
)

// Options controls the operation of Generate.
type Options struct {
// 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Fixed

}

// Generate generates Service Weaver code for the specified packages.
Expand All @@ -104,11 +115,13 @@ func Generate(dir string, pkgs []string, opt Options) error {
}
fset := token.NewFileSet()
cfg := &packages.Config{
Mode: packages.NeedName | packages.NeedSyntax | packages.NeedImports | packages.NeedTypes | packages.NeedTypesInfo,
Dir: dir,
Fset: fset,
ParseFile: parseNonWeaverGenFile,
BuildFlags: []string{"--tags=ignoreWeaverGen"},
Mode: packages.NeedName | packages.NeedSyntax | packages.NeedImports | packages.NeedTypes | packages.NeedTypesInfo,
Dir: dir,
Fset: fset,
ParseFile: parseNonWeaverGenFile,
}
if len(opt.BuildTags) > 0 {
cfg.BuildFlags = []string{"-tags", opt.BuildTags}
}
pkgList, err := packages.Load(cfg, pkgs...)
if err != nil {
Expand Down
58 changes: 53 additions & 5 deletions internal/tool/generate/generator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ replace github.com/ServiceWeaver/weaver => %s
//
// If "weaver generate" succeeds, the produced weaver_gen.go file is written in
// the provided directory with name ${filename}_weaver_gen.go.
func runGenerator(t *testing.T, directory, filename, contents string, subdirs []string) (string, error) {
func runGenerator(t *testing.T, directory, filename, contents string, subdirs []string,
buildTags []string) (string, error) {
// runGenerator creates a temporary directory, copies the file and all
// subdirs into it, writes a go.mod file, runs "go mod tidy", and finally
// runs "weaver generate".
Expand Down Expand Up @@ -102,7 +103,8 @@ func runGenerator(t *testing.T, directory, filename, contents string, subdirs []

// Run "weaver generate".
opt := Options{
Warn: func(err error) { t.Log(err) },
Warn: func(err error) { t.Log(err) },
BuildTags: "ignoreWeaverGen" + "," + strings.Join(buildTags, ","),
}
if err := Generate(tmp, []string{tmp}, opt); err != nil {
return "", err
Expand Down Expand Up @@ -134,7 +136,12 @@ func runGenerator(t *testing.T, directory, filename, contents string, subdirs []
if err := tidy.Run(); err != nil {
t.Fatalf("go mod tidy: %v", err)
}
gobuild := exec.Command("go", "build")

var buildTagsArg string
if len(buildTags) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the earlier code always added "ignoreWeaveGen". So can we supply -tags unconditionally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great observation. Fixed

buildTagsArg = "-tags=" + strings.Join(buildTags, ",")
}
gobuild := exec.Command("go", "build", buildTagsArg)
gobuild.Dir = tmp
gobuild.Stdout = os.Stdout
gobuild.Stderr = os.Stderr
Expand Down Expand Up @@ -218,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{})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just pass nil for the empty slice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if err != nil {
t.Fatalf("error running generator: %v", err)
}
Expand All @@ -237,6 +244,47 @@ 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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we use runGenerator for this test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

const dir = "testdata/tags"
files, err := os.ReadDir(dir)
if err != nil {
t.Fatalf("cannot list files in %q", dir)
}

for _, file := range files {
filename := file.Name()
if !strings.HasSuffix(filename, ".go") || strings.HasSuffix(filename, generatedCodeFile) {
continue
}
t.Run(filename, func(t *testing.T) {
t.Parallel()

// Read the test file.
bits, err := os.ReadFile(filepath.Join(dir, filename))
if err != nil {
t.Fatalf("cannot read %q: %v", filename, err)
}
contents := string(bits)
// Run "weaver generate".
output, err := runGenerator(t, dir, filename, contents, []string{}, []string{"good"})

if filename == "good.go" {
// Verify that the error is nil and the weaver_gen.go contains generated code for the good service.
if err != nil || !strings.Contains(output, "GoodService") {
t.Fatalf("expected generated code for the good service")
}
return
}
// For the bad.go verify that the error is not nil and there is no output.
if err == nil || len(output) > 0 {
t.Fatalf("expected no generated code for the good service")
}
})
}
}

// TestGeneratorErrors runs "weaver generate" on all of the files in
// testdata/errors.
// Every file in testdata/errors must begin with a single line header that looks
Expand Down Expand Up @@ -286,7 +334,7 @@ func TestGeneratorErrors(t *testing.T) {
}

// Run "weaver generate".
output, err := runGenerator(t, dir, filename, contents, []string{})
output, err := runGenerator(t, dir, filename, contents, []string{}, []string{})
errfile := strings.TrimSuffix(filename, ".go") + "_error.txt"
if err == nil {
os.Remove(filepath.Join(dir, errfile))
Expand Down
36 changes: 36 additions & 0 deletions internal/tool/generate/testdata/tags/bad.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Copyright 2024 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

//go:build !good

package tags

import (
"context"
"fmt"

"github.com/ServiceWeaver/weaver"
)

type BadService interface {
DoSomething(context.Context) error
}

type badServiceImpl struct {
weaver.Implements[BadService]
}

func (b *badServiceImpl) DoSomething(context.Context) error {
Some code that does not compile
}
37 changes: 37 additions & 0 deletions internal/tool/generate/testdata/tags/good.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Copyright 2024 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

//go:build good

package tags

import (
"context"
"fmt"

"github.com/ServiceWeaver/weaver"
)

type GoodService interface {
DoSomething(context.Context) error
}

type goodServiceImpl struct {
weaver.Implements[GoodService]
}

func (g *goodServiceImpl) DoSomething(context.Context) error {
fmt.Println("Hello world!")
return nil
}
Loading