Skip to content

Commit

Permalink
fileinfo: fix not detecting 'unix' files to be OS specific (#1554)
Browse files Browse the repository at this point in the history
The build file generation logic works like this today:

```
GenerateRules()
  package.go->*goTarget.addFiles
  package.go->getPlatformStringsAddFunction()
  fileinfo.go->isOSArchSpecific()
```

Depending on whether the file has special Go build directives, Gazelle
would use a different logic to include/exclude the file into the 'srcs'
attribute of the Go targets.

In #1512, we added support for 'unix' build directive but did not tell
Gazelle to treat files with 'unix' file as OS specific file. This caused
Gazelle to mismatch the OS in a later stage and exclude the needed file
instead.

Fix that logic and provide some additional tests to reinforce the
fileinfo logic.

Also added a small knob to print stdout of Gazelle when it's run under
go_repository with `debug_mode` attribute set to True.  Most Gazelle's
logs are printed to stdout and not stderr.
  • Loading branch information
sluongng authored May 31, 2023
1 parent 023974a commit e770d46
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 4 deletions.
3 changes: 2 additions & 1 deletion internal/go_repository.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,8 @@ def _go_repository_impl(ctx):
result.stderr,
))
if ctx.attr.debug_mode and result.stderr:
print("%s: %s" % (ctx.name, result.stderr))
print("%s gazelle.stdout: %s" % (ctx.name, result.stdout))
print("%s gazelle.stderr: %s" % (ctx.name, result.stderr))

# Apply patches if necessary.
patch(ctx)
Expand Down
3 changes: 1 addition & 2 deletions language/go/fileinfo.go
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,7 @@ func isOSArchSpecific(info fileInfo, cgoTags *cgoTagsAndOpts) (osSpecific, archS
checkTags := func(tags []string) {
for _, tag := range tags {
_, osOk := rule.KnownOSSet[tag]
if osOk {
if osOk || tag == "unix" {
osSpecific = true
}
_, archOk := rule.KnownArchSet[tag]
Expand Down Expand Up @@ -564,7 +564,6 @@ func checkConstraints(c *config.Config, os, arch, osSuffix, archSuffix string, t
return false
}
return matchesOS(os, tag)

}

if _, ok := rule.KnownArchSet[tag]; ok {
Expand Down
137 changes: 136 additions & 1 deletion language/go/fileinfo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ func TestOtherFileInfo(t *testing.T) {
if diff := cmp.Diff(tc.wantTags, got.tags, fileInfoCmpOption); diff != "" {
t.Errorf("(-want, +got): %s", diff)
}

})
}
}
Expand Down Expand Up @@ -427,6 +426,40 @@ func TestCheckConstraints(t *testing.T) {
os: "darwin",
arch: "amd64",
want: false,
}, {
desc: "unix filename on darwin",
filename: "foo_unix.go",
os: "darwin",
want: true,
}, {
desc: "unix filename on windows",
filename: "foo_unix.go",
os: "windows",
want: true,
}, {
desc: "non-unix tag on linux",
filename: "foo_bar.go",
os: "darwin",
content: "//go:build !unix\n\npackage foo",
want: false,
}, {
desc: "non-unix tag on windows",
filename: "foo_bar.go",
os: "windows",
content: "//go:build !unix\n\npackage foo",
want: true,
}, {
desc: "unix tag on windows",
filename: "foo_bar.go",
os: "windows",
content: "//go:build unix\n\npackage foo",
want: false,
}, {
desc: "unix tag on linux",
filename: "foo_bar.go",
os: "linux",
content: "//go:build unix\n\npackage foo",
want: true,
}, {
desc: "goos unsatisfied tags satisfied",
filename: "foo_linux.go",
Expand Down Expand Up @@ -580,3 +613,105 @@ import "C"
})
}
}

func TestIsOSArchSpecific(t *testing.T) {
for _, tc := range []struct {
desc string
filename, content string

expectOSSpecific bool
expectArchSpecific bool
}{
{
desc: "normal",
filename: "foo.go",
content: "package foo",
expectOSSpecific: false,
expectArchSpecific: false,
},
{
desc: "unix directive",
filename: "foo.go",
content: "//go:build unix\n\npackage foo",
expectOSSpecific: true,
expectArchSpecific: false,
},
{
desc: "exclude-unix directive",
filename: "foo.go",
content: "//go:build !unix\n\npackage foo",
expectOSSpecific: true,
expectArchSpecific: false,
},
{
desc: "arch directive",
filename: "foo.go",
content: "//go:build arm64\n\npackage foo",
expectOSSpecific: false,
expectArchSpecific: true,
},
{
desc: "exclude-arch directive",
filename: "foo.go",
content: "//go:build !arm64\n\npackage foo",
expectOSSpecific: false,
expectArchSpecific: true,
},
{
desc: "os directive",
filename: "foo.go",
content: "//go:build linux\n\npackage foo",
expectOSSpecific: true,
expectArchSpecific: false,
},
{
desc: "exclude-os directive",
filename: "foo.go",
content: "//go:build !linux\n\npackage foo",
expectOSSpecific: true,
expectArchSpecific: false,
},
{
desc: "os and arch directive",
filename: "foo.go",
content: "//go:build linux && amd64\n\npackage foo",
expectOSSpecific: true,
expectArchSpecific: true,
},
{
desc: "unix and arch directive",
filename: "foo.go",
content: "//go:build unix && amd64\n\npackage foo",
expectOSSpecific: true,
expectArchSpecific: true,
},
} {
t.Run(tc.desc, func(t *testing.T) {
tmpDir, err := os.MkdirTemp(os.Getenv("TEST_TEMPDIR"), "TestIsOSSpecific_*")
if err != nil {
t.Fatal(err)
}
t.Cleanup(func() {
os.RemoveAll(tmpDir)
})

path := filepath.Join(tmpDir, tc.filename)
if err := ioutil.WriteFile(path, []byte(tc.content), 0o666); err != nil {
t.Fatal(err)
}
fi := goFileInfo(path, "")
var cgoTags *cgoTagsAndOpts
if len(fi.copts) > 0 {
cgoTags = fi.copts[0]
}

gotOSSpecific, gotArchSpecific := isOSArchSpecific(fi, cgoTags)
if diff := cmp.Diff(tc.expectOSSpecific, gotOSSpecific); diff != "" {
t.Errorf("(-want, +got): %s", diff)
}
if diff := cmp.Diff(tc.expectArchSpecific, gotArchSpecific); diff != "" {
t.Errorf("(-want, +got): %s", diff)
}
})
}
}

0 comments on commit e770d46

Please sign in to comment.