From ef107821b732236d052d7ec2bd545274217871ec Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 23 Aug 2022 11:51:09 +0200 Subject: [PATCH 1/5] golang: add tests Signed-off-by: Sebastiaan van Stijn --- golang_test.go | 78 +++++++++++++++++++++++++++++++++++++++ testdata/go.basic | 1 + testdata/go.basic.golden | 17 +++++++++ testdata/templates/go.txt | 16 ++++++++ 4 files changed, 112 insertions(+) create mode 100644 golang_test.go create mode 100644 testdata/go.basic create mode 100644 testdata/go.basic.golden create mode 100644 testdata/templates/go.txt diff --git a/golang_test.go b/golang_test.go new file mode 100644 index 0000000..404399d --- /dev/null +++ b/golang_test.go @@ -0,0 +1,78 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestGolangApplier_ApplyHeader(t *testing.T) { + tc := newGolangTagContext(t) + defer func() { _ = tc.templateFiles.dTemplateFile.Close() }() + + tmpDir := t.TempDir() + + files := []string{ + "go.basic", + } + + g := golangApplier{} + for _, f := range files { + fileName := f + t.Run(fileName, func(t *testing.T) { + testfile := filepath.Join(tmpDir, fileName) + copyFile(t, "./testdata/"+fileName, testfile) + + err := g.ApplyHeader(testfile, &tc) + if err != nil { + t.Fatalf("failed to apply header to %s: %+v", testfile, err) + } + compareFiles(t, testfile, "./testdata/"+fileName+".golden") + }) + } +} + +func TestGolangApplier_CheckHeader(t *testing.T) { + files := []string{ + // The non-golden files don't have a header present + "go.basic", + + // The golden files should have the header present + "go.basic.golden", + } + + g := golangApplier{} + tc := newGolangTagContext(t) + defer func() { _ = tc.templateFiles.dTemplateFile.Close() }() + for _, f := range files { + fileName := f + t.Run(fileName, func(t *testing.T) { + f, err := os.Open("./testdata/" + fileName) + if err != nil { + t.Fatalf("failed to optn file %s: %+v", fileName, err) + } + defer func() { _ = f.Close() }() + found, err := g.CheckHeader(f, &tc) + if err != nil { + t.Fatalf("failed to check header: %+v", err) + } + expected := strings.HasSuffix(fileName, ".golden") + if found != expected { + t.Fail() + } + }) + } +} + +func newGolangTagContext(t *testing.T) TagContext { + t.Helper() + templateFile, err := loadTemplate("./testdata/templates/", "go.txt") + if err != nil { + t.Fatalf("failed to load Go template") + } + return TagContext{ + templateFiles: TemplateFiles{goTemplateFile: templateFile}, + templatePath: "./testdata/templates/", + } +} diff --git a/testdata/go.basic b/testdata/go.basic new file mode 100644 index 0000000..b0736c3 --- /dev/null +++ b/testdata/go.basic @@ -0,0 +1 @@ +package plugin diff --git a/testdata/go.basic.golden b/testdata/go.basic.golden new file mode 100644 index 0000000..d5627b8 --- /dev/null +++ b/testdata/go.basic.golden @@ -0,0 +1,17 @@ +/* + Copyright + + 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. +*/ + +package plugin diff --git a/testdata/templates/go.txt b/testdata/templates/go.txt new file mode 100644 index 0000000..8f80817 --- /dev/null +++ b/testdata/templates/go.txt @@ -0,0 +1,16 @@ +/* + Copyright + + 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. +*/ + From 1dc490938f961d12075be35c24638f059e580bd0 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 23 Aug 2022 12:37:15 +0200 Subject: [PATCH 2/5] golang: return early on generated files Signed-off-by: Sebastiaan van Stijn --- golang.go | 15 ++++++----- golang_test.go | 49 +++++++++++++++++++++++------------- testdata/go.generated | 7 ++++++ testdata/go.generated.golden | 7 ++++++ 4 files changed, 52 insertions(+), 26 deletions(-) create mode 100644 testdata/go.generated create mode 100644 testdata/go.generated.golden diff --git a/golang.go b/golang.go index d040b74..96df2f1 100644 --- a/golang.go +++ b/golang.go @@ -20,15 +20,15 @@ func (g *golangApplier) CheckHeader(target *os.File, t *TagContext) (bool, error } target.Seek(0, 0) + if cFlags == AutoGenerated { + return true, nil + } + tbuf, err := os.ReadFile(filepath.Join(t.templatePath, "go.txt")) if err != nil { return false, err } - if cFlags == AutoGenerated { - return true, nil - } - var templateBuf string if cFlags == CompilerFlags { for _, cbuf := range cbufs { @@ -130,6 +130,9 @@ func (g *golangApplier) checkSpecialConditions(target *os.File) (uint8, [][]byte if err != nil { return NormalFiles, nil, err } + if strings.HasPrefix(string(buf), "//") && strings.Contains(string(buf), "DO NOT EDIT") { + return AutoGenerated, nil, nil + } // Go 1.17 compiler flags (e.g., `//go:build !windows`) if strings.HasPrefix(string(buf), "//go:") { @@ -152,9 +155,5 @@ func (g *golangApplier) checkSpecialConditions(target *os.File) (uint8, [][]byte !strings.Contains(string(buf), "Package") { return CompilerFlags, [][]byte{buf}, nil } - if strings.HasPrefix(string(buf), "//") && - (strings.Contains(string(buf), "DO NOT EDIT")) { - return AutoGenerated, nil, nil - } return NormalFiles, nil, nil } diff --git a/golang_test.go b/golang_test.go index 404399d..161bb4d 100644 --- a/golang_test.go +++ b/golang_test.go @@ -3,7 +3,6 @@ package main import ( "os" "path/filepath" - "strings" "testing" ) @@ -15,6 +14,7 @@ func TestGolangApplier_ApplyHeader(t *testing.T) { files := []string{ "go.basic", + "go.generated", } g := golangApplier{} @@ -34,31 +34,44 @@ func TestGolangApplier_ApplyHeader(t *testing.T) { } func TestGolangApplier_CheckHeader(t *testing.T) { - files := []string{ - // The non-golden files don't have a header present - "go.basic", - - // The golden files should have the header present - "go.basic.golden", + tests := []struct { + fileName string + expected bool + }{ + { + fileName: "go.basic", + expected: false, + }, + { + fileName: "go.generated", + expected: true, // Generated files are not checked, and always "ok" + }, + { + fileName: "go.basic.golden", + expected: true, // Generated files are not checked, and always "ok" + }, + { + fileName: "go.generated.golden", + expected: true, // Generated files are not checked, and always "ok" + }, } - g := golangApplier{} - tc := newGolangTagContext(t) - defer func() { _ = tc.templateFiles.dTemplateFile.Close() }() - for _, f := range files { - fileName := f - t.Run(fileName, func(t *testing.T) { - f, err := os.Open("./testdata/" + fileName) + tagContext := newGolangTagContext(t) + defer func() { _ = tagContext.templateFiles.dTemplateFile.Close() }() + for _, tc := range tests { + tc := tc + t.Run(tc.fileName, func(t *testing.T) { + f, err := os.Open("./testdata/" + tc.fileName) if err != nil { - t.Fatalf("failed to optn file %s: %+v", fileName, err) + t.Fatalf("failed to open file %s: %+v", tc.fileName, err) } defer func() { _ = f.Close() }() - found, err := g.CheckHeader(f, &tc) + found, err := g.CheckHeader(f, &tagContext) + if err != nil { t.Fatalf("failed to check header: %+v", err) } - expected := strings.HasSuffix(fileName, ".golden") - if found != expected { + if found != tc.expected { t.Fail() } }) diff --git a/testdata/go.generated b/testdata/go.generated new file mode 100644 index 0000000..d414231 --- /dev/null +++ b/testdata/go.generated @@ -0,0 +1,7 @@ +// Code generated by protoc-gen-go. DO NOT EDIT. +// versions: +// protoc-gen-go v1.28.1 +// protoc v3.20.1 +// source: github.com/containerd/containerd/api/types/metrics.proto + +package types diff --git a/testdata/go.generated.golden b/testdata/go.generated.golden new file mode 100644 index 0000000..d414231 --- /dev/null +++ b/testdata/go.generated.golden @@ -0,0 +1,7 @@ +// Code generated by protoc-gen-go. DO NOT EDIT. +// versions: +// protoc-gen-go v1.28.1 +// protoc v3.20.1 +// source: github.com/containerd/containerd/api/types/metrics.proto + +package types From 6315543c6853af2dc88de54a4607e596b1a56753 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 23 Aug 2022 13:40:43 +0200 Subject: [PATCH 3/5] golang: checkSpecialConditions: return string-slice for convenience Signed-off-by: Sebastiaan van Stijn --- golang.go | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/golang.go b/golang.go index 96df2f1..eab9812 100644 --- a/golang.go +++ b/golang.go @@ -31,9 +31,7 @@ func (g *golangApplier) CheckHeader(target *os.File, t *TagContext) (bool, error var templateBuf string if cFlags == CompilerFlags { - for _, cbuf := range cbufs { - templateBuf += string(cbuf) + "\n" - } + templateBuf = strings.Join(cbufs, "\n") templateBuf += "\n" templateBuf += string(tbuf) } else { @@ -95,8 +93,7 @@ func (g *golangApplier) ApplyHeader(path string, t *TagContext) error { reader := bufio.NewReader(file) if sFlags == CompilerFlags { for _, f := range flags { - tFile.Write(f) - tFile.Write([]byte("\n")) + tFile.WriteString(f + "\n") _, _, err = reader.ReadLine() } _, _, err = reader.ReadLine() @@ -124,36 +121,38 @@ func (g *golangApplier) ApplyHeader(path string, t *TagContext) error { return nil } -func (g *golangApplier) checkSpecialConditions(target *os.File) (uint8, [][]byte, error) { +func (g *golangApplier) checkSpecialConditions(target *os.File) (uint8, []string, error) { reader := bufio.NewReader(target) buf, _, err := reader.ReadLine() if err != nil { return NormalFiles, nil, err } - if strings.HasPrefix(string(buf), "//") && strings.Contains(string(buf), "DO NOT EDIT") { + line := string(buf) + + if strings.HasPrefix(line, "//") && strings.Contains(line, "DO NOT EDIT") { return AutoGenerated, nil, nil } // Go 1.17 compiler flags (e.g., `//go:build !windows`) - if strings.HasPrefix(string(buf), "//go:") { + if strings.HasPrefix(line, "//go:") { // read next line too: (`// +build !windows`) if buf2, _, err := reader.ReadLine(); err == nil && strings.HasPrefix(string(buf2), "// +build ") { - return CompilerFlags, [][]byte{buf, buf2}, nil + return CompilerFlags, []string{line, string(buf2)}, nil } - return CompilerFlags, [][]byte{buf}, nil + return CompilerFlags, []string{line}, nil } // Old compiler flags (e.g., `// +build !windows`) // checks for Package comments as per https://blog.golang.org/godoc-documenting-go-code - if strings.HasPrefix(string(buf), "//") && - (strings.Contains(string(buf), "build") || - strings.Contains(string(buf), "unix") || - strings.Contains(string(buf), "linux") || - strings.Contains(string(buf), "windows") || - strings.Contains(string(buf), "darwin") || - strings.Contains(string(buf), "freebsd")) && - !strings.Contains(string(buf), "Package") { - return CompilerFlags, [][]byte{buf}, nil + if strings.HasPrefix(line, "//") && + (strings.Contains(line, "build") || + strings.Contains(line, "unix") || + strings.Contains(line, "linux") || + strings.Contains(line, "windows") || + strings.Contains(line, "darwin") || + strings.Contains(line, "freebsd")) && + !strings.Contains(line, "Package") { + return CompilerFlags, []string{line}, nil } return NormalFiles, nil, nil } From 13298729b86c2ab703830d940720b5f7788d4cb6 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 23 Aug 2022 13:03:43 +0200 Subject: [PATCH 4/5] golang: add whitespace between build-tags and header This looks to be how it's used in current projects; also add a test- case with a single build-tag. Signed-off-by: Sebastiaan van Stijn --- golang.go | 3 ++- golang_test.go | 9 +++++++++ testdata/go.single-buildtag | 4 ++++ testdata/go.single-buildtag.golden | 20 ++++++++++++++++++++ 4 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 testdata/go.single-buildtag create mode 100644 testdata/go.single-buildtag.golden diff --git a/golang.go b/golang.go index eab9812..505a947 100644 --- a/golang.go +++ b/golang.go @@ -31,7 +31,7 @@ func (g *golangApplier) CheckHeader(target *os.File, t *TagContext) (bool, error var templateBuf string if cFlags == CompilerFlags { - templateBuf = strings.Join(cbufs, "\n") + templateBuf = strings.Join(cbufs, "\n") + "\n" templateBuf += "\n" templateBuf += string(tbuf) } else { @@ -96,6 +96,7 @@ func (g *golangApplier) ApplyHeader(path string, t *TagContext) error { tFile.WriteString(f + "\n") _, _, err = reader.ReadLine() } + tFile.WriteString("\n") _, _, err = reader.ReadLine() } diff --git a/golang_test.go b/golang_test.go index 161bb4d..2272ed7 100644 --- a/golang_test.go +++ b/golang_test.go @@ -15,6 +15,7 @@ func TestGolangApplier_ApplyHeader(t *testing.T) { files := []string{ "go.basic", "go.generated", + "go.single-buildtag", } g := golangApplier{} @@ -46,6 +47,10 @@ func TestGolangApplier_CheckHeader(t *testing.T) { fileName: "go.generated", expected: true, // Generated files are not checked, and always "ok" }, + { + fileName: "go.single-buildtag", + expected: false, + }, { fileName: "go.basic.golden", expected: true, // Generated files are not checked, and always "ok" @@ -54,6 +59,10 @@ func TestGolangApplier_CheckHeader(t *testing.T) { fileName: "go.generated.golden", expected: true, // Generated files are not checked, and always "ok" }, + { + fileName: "go.single-buildtag.golden", + expected: true, + }, } g := golangApplier{} tagContext := newGolangTagContext(t) diff --git a/testdata/go.single-buildtag b/testdata/go.single-buildtag new file mode 100644 index 0000000..80249da --- /dev/null +++ b/testdata/go.single-buildtag @@ -0,0 +1,4 @@ +//go:build amd64 +// +build amd64 + +package main diff --git a/testdata/go.single-buildtag.golden b/testdata/go.single-buildtag.golden new file mode 100644 index 0000000..c1d6348 --- /dev/null +++ b/testdata/go.single-buildtag.golden @@ -0,0 +1,20 @@ +//go:build amd64 +// +build amd64 + +/* + Copyright + + 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. +*/ + +package main From 9f815290f682f7c1b9c876d86f9ecef9d3700b77 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 23 Aug 2022 14:09:20 +0200 Subject: [PATCH 5/5] golang: fix files with multiple build-tags Signed-off-by: Sebastiaan van Stijn --- golang.go | 36 +++++++++++--------- golang_test.go | 18 ++++++++++ testdata/go.multiple-buildtags | 7 ++++ testdata/go.multiple-buildtags-legacy | 6 ++++ testdata/go.multiple-buildtags-legacy.golden | 22 ++++++++++++ testdata/go.multiple-buildtags.golden | 23 +++++++++++++ 6 files changed, 95 insertions(+), 17 deletions(-) create mode 100644 testdata/go.multiple-buildtags create mode 100644 testdata/go.multiple-buildtags-legacy create mode 100644 testdata/go.multiple-buildtags-legacy.golden create mode 100644 testdata/go.multiple-buildtags.golden diff --git a/golang.go b/golang.go index 505a947..7bc342b 100644 --- a/golang.go +++ b/golang.go @@ -134,26 +134,28 @@ func (g *golangApplier) checkSpecialConditions(target *os.File) (uint8, []string return AutoGenerated, nil, nil } - // Go 1.17 compiler flags (e.g., `//go:build !windows`) - if strings.HasPrefix(line, "//go:") { + var buildTags []string + + // Go 1.17 compiler flags (e.g., `//go:build !windows`), or old compiler + // flags (e.g., `// +build !windows`). + // checks for Package comments as per https://blog.golang.org/godoc-documenting-go-code + if strings.HasPrefix(line, "//go:") || strings.HasPrefix(line, "// +build") { + buildTags = append(buildTags, line) + // read next line too: (`// +build !windows`) - if buf2, _, err := reader.ReadLine(); err == nil && strings.HasPrefix(string(buf2), "// +build ") { - return CompilerFlags, []string{line, string(buf2)}, nil + for { + buf2, _, err := reader.ReadLine() + if err != nil { + break + } + if strings.HasPrefix(string(buf2), "// +build ") { + buildTags = append(buildTags, string(buf2)) + } else { + break + } } - return CompilerFlags, []string{line}, nil + return CompilerFlags, buildTags, nil } - // Old compiler flags (e.g., `// +build !windows`) - // checks for Package comments as per https://blog.golang.org/godoc-documenting-go-code - if strings.HasPrefix(line, "//") && - (strings.Contains(line, "build") || - strings.Contains(line, "unix") || - strings.Contains(line, "linux") || - strings.Contains(line, "windows") || - strings.Contains(line, "darwin") || - strings.Contains(line, "freebsd")) && - !strings.Contains(line, "Package") { - return CompilerFlags, []string{line}, nil - } return NormalFiles, nil, nil } diff --git a/golang_test.go b/golang_test.go index 2272ed7..c0266a4 100644 --- a/golang_test.go +++ b/golang_test.go @@ -16,6 +16,8 @@ func TestGolangApplier_ApplyHeader(t *testing.T) { "go.basic", "go.generated", "go.single-buildtag", + "go.multiple-buildtags", + "go.multiple-buildtags-legacy", } g := golangApplier{} @@ -51,6 +53,14 @@ func TestGolangApplier_CheckHeader(t *testing.T) { fileName: "go.single-buildtag", expected: false, }, + { + fileName: "go.multiple-buildtags", + expected: false, + }, + { + fileName: "go.multiple-buildtags-legacy", + expected: false, + }, { fileName: "go.basic.golden", expected: true, // Generated files are not checked, and always "ok" @@ -63,6 +73,14 @@ func TestGolangApplier_CheckHeader(t *testing.T) { fileName: "go.single-buildtag.golden", expected: true, }, + { + fileName: "go.multiple-buildtags.golden", + expected: true, + }, + { + fileName: "go.multiple-buildtags-legacy.golden", + expected: true, + }, } g := golangApplier{} tagContext := newGolangTagContext(t) diff --git a/testdata/go.multiple-buildtags b/testdata/go.multiple-buildtags new file mode 100644 index 0000000..ddfae86 --- /dev/null +++ b/testdata/go.multiple-buildtags @@ -0,0 +1,7 @@ +//go:build (amd64 || arm64) && !static_build && !gccgo +// +build amd64 arm64 +// +build !static_build +// +build !gccgo + +// Package plugin does something. +package plugin diff --git a/testdata/go.multiple-buildtags-legacy b/testdata/go.multiple-buildtags-legacy new file mode 100644 index 0000000..9cfb61c --- /dev/null +++ b/testdata/go.multiple-buildtags-legacy @@ -0,0 +1,6 @@ +// +build amd64 arm64 +// +build !static_build +// +build !gccgo + +// Package plugin does something. +package plugin diff --git a/testdata/go.multiple-buildtags-legacy.golden b/testdata/go.multiple-buildtags-legacy.golden new file mode 100644 index 0000000..3e5d903 --- /dev/null +++ b/testdata/go.multiple-buildtags-legacy.golden @@ -0,0 +1,22 @@ +// +build amd64 arm64 +// +build !static_build +// +build !gccgo + +/* + Copyright + + 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. +*/ + +// Package plugin does something. +package plugin diff --git a/testdata/go.multiple-buildtags.golden b/testdata/go.multiple-buildtags.golden new file mode 100644 index 0000000..22ad0ff --- /dev/null +++ b/testdata/go.multiple-buildtags.golden @@ -0,0 +1,23 @@ +//go:build (amd64 || arm64) && !static_build && !gccgo +// +build amd64 arm64 +// +build !static_build +// +build !gccgo + +/* + Copyright + + 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. +*/ + +// Package plugin does something. +package plugin