Merge pull request #2388 from monopole/removeStaticCling

Remove static cling in plugin development flow.

Makefile

@@ -188,8 +188,14 @@ lint-kustomize: install-tools $(builtinplugins)
 	cd pluginator; \
 	$(MYGOBIN)/golangci-lint-kustomize -c ../.golangci-kustomize.yml run ./...
 
+# Used to add non-default compilation flags when experimenting with
+# plugin-to-api compatibility checks.
+.PHONY: build-kustomize-api
+build-kustomize-api: $(builtinplugins)
+	cd api; go build ./...
+
 .PHONY: test-unit-kustomize-api
-test-unit-kustomize-api: $(builtinplugins)
+test-unit-kustomize-api: build-kustomize-api
 	cd api; go test ./...
 
 .PHONY: test-unit-kustomize-plugins
@@ -288,6 +294,7 @@ clean: kustomize-external-go-plugin-clean
 	rm -f $(MYGOBIN)/kustomize
 	rm -f $(MYGOBIN)/golangci-lint-kustomize
 
+# Nuke the site from orbit.  It's the only way to be sure.
 .PHONY: nuke
 nuke: clean
-	sudo rm -rf $(shell go env GOPATH)/pkg/mod/sigs.k8s.io
+	go clean --modcache

api/internal/plugins/compiler/compiler.go

@@ -6,6 +6,7 @@ package compiler
 import (
 	"bytes"
 	"fmt"
+	"log"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -16,61 +17,74 @@ import (
 )
 
 // Compiler creates Go plugin object files.
-//
-// Source code is read from
-//   ${srcRoot}/${g}/${v}/${k}.go
-//
-// Object code is written to
-//   ${objRoot}/${g}/${v}/${k}.so
 type Compiler struct {
-	srcRoot string
+	// pluginRoot is where the user
-	objRoot string
+	// has her ${g}/${v}/$lower(${k})/${k}.go files.
+	pluginRoot string
+	// Where compilation happens.
+	workDir string
+	// Used as the root file name for src and object.
+	rawKind string
+	// Capture compiler output.
+	stderr bytes.Buffer
+	// Capture compiler output.
+	stdout bytes.Buffer
 }
 
 // NewCompiler returns a new compiler instance.
-func NewCompiler(srcRoot, objRoot string) *Compiler {
+func NewCompiler(root string) *Compiler {
-	return &Compiler{srcRoot: srcRoot, objRoot: objRoot}
+	return &Compiler{pluginRoot: root}
 }
 
-// ObjRoot is root of compilation target tree.
+// Set GVK converts g,v,k tuples to file path components.
-func (b *Compiler) ObjRoot() string {
+func (b *Compiler) SetGVK(g, v, k string) {
-	return b.objRoot
+	b.rawKind = k
+	b.workDir = filepath.Join(b.pluginRoot, g, v, strings.ToLower(k))
 }
 
-// SrcRoot is where to find src.
+func (b *Compiler) srcPath() string {
-func (b *Compiler) SrcRoot() string {
+	return filepath.Join(b.workDir, b.rawKind+".go")
-	return b.srcRoot
 }
 
-// Compile reads ${srcRoot}/${g}/${v}/${k}.go
+func (b *Compiler) objFile() string {
-//    and writes ${objRoot}/${g}/${v}/${k}.so
+	return b.rawKind + ".so"
-func (b *Compiler) Compile(g, v, k string) error {
+}
-	lowK := strings.ToLower(k)
+
-	objDir := filepath.Join(b.objRoot, g, v, lowK)
+// Absolute path to the compiler output (the .so file).
-	objFile := filepath.Join(objDir, k) + ".so"
+func (b *Compiler) ObjPath() string {
-	if FileYoungerThan(objFile, time.Minute) {
+	return filepath.Join(b.workDir, b.objFile())
-		// Skip rebuilding it.
+}
+
+// Cleanup provides a hook to delete the .so file.
+// Ignore errors.
+func (b *Compiler) Cleanup() {
+	_ = os.Remove(b.ObjPath())
+}
+
+// Compile changes its working directory to
+// ${pluginRoot}/${g}/${v}/$lower(${k} and places
+// object code next to source code.
+func (b *Compiler) Compile() error {
+	if FileYoungerThan(b.ObjPath(), 8*time.Second) {
+		// Skip rebuilding it, to save time in a plugin test file
+		// that has many distinct calls to make a harness and compile
+		// the plugin (only the first compile will happen).
+		// Make it a short time to avoid tricking someone who's actively
+		// developing a plugin.
 		return nil
 	}
-	err := os.MkdirAll(objDir, os.ModePerm)
+	if !FileExists(b.srcPath()) {
-	if err != nil {
+		return fmt.Errorf("cannot  find source at '%s'", b.srcPath())
-		return err
-	}
-	srcFile := filepath.Join(b.srcRoot, g, v, lowK, k) + ".go"
-	if !FileExists(srcFile) {
-		// Handy for tests of lone plugins.
-		s := k + ".go"
-		if !FileExists(s) {
-			return fmt.Errorf(
-				"cannot find source at '%s' or '%s'", srcFile, s)
-		}
-		srcFile = s
 	}
+	// If you use an IDE, make sure it's go build and test flags
+	// match those used below.  Same goes for Makefile targets.
 	commands := []string{
 		"build",
+		// "-trimpath",  This flag used to make it better, now it makes it worse,
+		//               see https://github.com/golang/go/issues/31354
 		"-buildmode",
 		"plugin",
-		"-o", objFile, srcFile,
+		"-o", b.objFile(),
 	}
 	goBin := goBin()
 	if !FileExists(goBin) {
@@ -78,12 +92,26 @@ func (b *Compiler) Compile(g, v, k string) error {
 			"cannot find go compiler %s", goBin)
 	}
 	cmd := exec.Command(goBin, commands...)
-	var stderr bytes.Buffer
+	b.stderr.Reset()
-	cmd.Stderr = &stderr
+	cmd.Stderr = &b.stderr
+	b.stdout.Reset()
+	cmd.Stdout = &b.stdout
 	cmd.Env = os.Environ()
+	cmd.Dir = b.workDir
 	if err := cmd.Run(); err != nil {
+		b.report()
 		return errors.Wrapf(
-			err, "cannot compile %s:\nSTDERR\n%s\n", srcFile, stderr.String())
+			err, "cannot compile %s:\nSTDERR\n%s\n",
+			b.srcPath(), b.stderr.String())
 	}
 	return nil
 }
+
+func (b *Compiler) report() {
+	log.Println("stdout:  -------")
+	log.Println(b.stdout.String())
+	log.Println("----------------")
+	log.Println("stderr:  -------")
+	log.Println(b.stderr.String())
+	log.Println("----------------")
+}

api/internal/plugins/compiler/compiler_test.go

@@ -4,8 +4,6 @@
 package compiler_test
 
 import (
-	"io/ioutil"
-	"os"
 	"path/filepath"
 	"testing"
 	"time"
@@ -16,53 +14,46 @@ import (
 
 // Regression coverage over compiler behavior.
 func TestCompiler(t *testing.T) {
-	configRoot, err := ioutil.TempDir("", "kustomize-compiler-test")
-	if err != nil {
-		t.Errorf("failed to make temp dir: %v", err)
-	}
 	srcRoot, err := DeterminePluginSrcRoot(filesys.MakeFsOnDisk())
 	if err != nil {
 		t.Error(err)
 	}
-	c := NewCompiler(srcRoot, configRoot)
+	c := NewCompiler(srcRoot)
-	if configRoot != c.ObjRoot() {
-		t.Errorf("unexpected objRoot %s", c.ObjRoot())
-	}
 
+	c.SetGVK("someteam.example.com", "v1", "DatePrefixer")
 	expectObj := filepath.Join(
-		c.ObjRoot(),
+		srcRoot, "someteam.example.com", "v1", "dateprefixer", "DatePrefixer.so")
-		"someteam.example.com", "v1", "dateprefixer", "DatePrefixer.so")
+	if expectObj != c.ObjPath() {
-	if FileExists(expectObj) {
+		t.Errorf("Expected '%s', got '%s'", expectObj, c.ObjPath())
-		t.Errorf("obj file should not exist yet: %s", expectObj)
 	}
-	err = c.Compile("someteam.example.com", "v1", "DatePrefixer")
+	err = c.Compile()
 	if err != nil {
 		t.Error(err)
 	}
 	if !FileYoungerThan(expectObj, time.Second) {
 		t.Errorf("didn't find expected obj file %s", expectObj)
 	}
+	c.Cleanup()
+	if FileExists(expectObj) {
+		t.Errorf("obj file '%s' should be gone", expectObj)
+	}
 
+	c.SetGVK("builtin", "", "SecretGenerator")
 	expectObj = filepath.Join(
-		c.ObjRoot(),
+		srcRoot,
 		"builtin", "", "secretgenerator", "SecretGenerator.so")
-	if FileExists(expectObj) {
+	if expectObj != c.ObjPath() {
-		t.Errorf("obj file should not exist yet: %s", expectObj)
+		t.Errorf("Expected '%s', got '%s'", expectObj, c.ObjPath())
 	}
-	err = c.Compile("builtin", "", "SecretGenerator")
+	err = c.Compile()
 	if err != nil {
 		t.Error(err)
 	}
 	if !FileYoungerThan(expectObj, time.Second) {
 		t.Errorf("didn't find expected obj file %s", expectObj)
 	}
-
+	c.Cleanup()
-	err = os.RemoveAll(c.ObjRoot())
-	if err != nil {
-		t.Errorf(
-			"removing temp dir: %s %v", c.ObjRoot(), err)
-	}
 	if FileExists(expectObj) {
-		t.Errorf("cleanup failed; still see: %s", expectObj)
+		t.Errorf("obj file '%s' should be gone", expectObj)
 	}
 }

api/testutils/kusttest/harness.go

@@ -70,8 +70,7 @@ func (th Harness) MakeOptionsPluginsDisabled() krusty.Options {
 
 // Enables use of non-builtin plugins.
 func (th Harness) MakeOptionsPluginsEnabled() krusty.Options {
-	// TODO: Change to types.BploLoadFromFileSys to enable debugging.
+	c, err := konfig.EnabledPluginConfig(types.BploLoadFromFileSys)
-	c, err := konfig.EnabledPluginConfig(types.BploUseStaticallyLinked)
 	if err != nil {
 		if strings.Contains(err.Error(), "unable to find plugin root") {
 			th.t.Log(

api/testutils/kusttest/harnessenhanced.go

@@ -47,8 +47,7 @@ type HarnessEnhanced struct {
 func MakeEnhancedHarness(t *testing.T) *HarnessEnhanced {
 	pte := newPluginTestEnv(t).set()
 
-	// TODO: Change to types.BploLoadFromFileSys to enable debugging.
+	pc, err := konfig.EnabledPluginConfig(types.BploLoadFromFileSys)
-	pc, err := konfig.EnabledPluginConfig(types.BploUseStaticallyLinked)
 	if err != nil {
 		t.Fatal(err)
 	}

api/testutils/kusttest/plugintestenv.go

@@ -4,11 +4,7 @@
 package kusttest_test
 
 import (
-	"io/ioutil"
 	"os"
-	"os/exec"
-	"path/filepath"
-	"strings"
 	"testing"
 
 	"sigs.k8s.io/kustomize/api/filesys"
@@ -17,16 +13,13 @@ import (
 )
 
 // pluginTestEnv manages compiling plugins for tests.
-// It manages a Go plugin compiler,
+// It manages a Go plugin compiler, and sets/resets shell env vars as needed.
-// maybe makes and removes a temporary working directory,
-// maybe sets/resets shell env vars as needed.
 type pluginTestEnv struct {
-	t        *testing.T
+	t          *testing.T
-	compiler *compiler.Compiler
+	compiler   *compiler.Compiler
-	srcRoot  string
+	pluginRoot string
-	workDir  string
+	oldXdg     string
-	oldXdg   string
+	wasSet     bool
-	wasSet   bool
 }
 
 // newPluginTestEnv returns a new instance of pluginTestEnv.
@@ -39,76 +32,44 @@ func newPluginTestEnv(t *testing.T) *pluginTestEnv {
 // plugin code - this FileSystem has nothing to do with
 // the FileSystem used for loading config yaml in the tests.
 func (x *pluginTestEnv) set() *pluginTestEnv {
-	x.createWorkDir()
 	var err error
-	x.srcRoot, err = compiler.DeterminePluginSrcRoot(filesys.MakeFsOnDisk())
+	x.pluginRoot, err = compiler.DeterminePluginSrcRoot(filesys.MakeFsOnDisk())
 	if err != nil {
 		x.t.Error(err)
 	}
-	x.compiler = compiler.NewCompiler(x.srcRoot, x.workDir)
+	x.compiler = compiler.NewCompiler(x.pluginRoot)
 	x.setEnv()
 	return x
 }
 
 // reset restores the environment to pre-test state.
 func (x *pluginTestEnv) reset() {
+	// Calling Cleanup forces recompilation in a test file with multiple
+	// calls to MakeEnhancedHarness - so leaving it out.  Your .gitignore
+	// should ignore .so files anyway.
+	// x.compiler.Cleanup()
 	x.resetEnv()
-	x.removeWorkDir()
 }
 
 // prepareGoPlugin compiles a Go plugin, leaving the newly
-// created object code in the right place - a temporary
+// created object code alongside the src code.
-// working directory pointed to by KustomizePluginHomeEnv.
-// This avoids overwriting anything the user/developer has
-// otherwise created.
 func (x *pluginTestEnv) prepareGoPlugin(g, v, k string) {
-	err := x.compiler.Compile(g, v, k)
+	x.compiler.SetGVK(g, v, k)
+	err := x.compiler.Compile()
 	if err != nil {
 		x.t.Errorf("compile failed: %v", err)
 	}
 }
 
-// prepareExecPlugin copies an exec plugin from it's
+func (x *pluginTestEnv) prepareExecPlugin(_, _, _ string) {
-// home in the discovered srcRoot to the same temp
+	// Do nothing.  At one point this method
-// directory where Go plugin object code is placed.
+	// copied the exec plugin directory to a temp dir
-// Kustomize (and its tests) expect to find plugins
+	// and ran it from there.  Left as a hook.
-// (Go or Exec) in the same spot, and since the test
-// framework is compiling Go plugins to a temp dir,
-// it must likewise copy Exec plugins to that same
-// temp dir.
-func (x *pluginTestEnv) prepareExecPlugin(g, v, k string) {
-	lowK := strings.ToLower(k)
-	src := filepath.Join(x.srcRoot, g, v, lowK, k)
-	tmp := filepath.Join(x.workDir, g, v, lowK, k)
-	if err := os.MkdirAll(filepath.Dir(tmp), 0755); err != nil {
-		x.t.Errorf("error making directory: %s", filepath.Dir(tmp))
-	}
-	cmd := exec.Command("cp", src, tmp)
-	cmd.Env = os.Environ()
-	if err := cmd.Run(); err != nil {
-		x.t.Errorf("error copying %s to %s: %v", src, tmp, err)
-	}
-}
-
-func (x *pluginTestEnv) createWorkDir() {
-	var err error
-	x.workDir, err = ioutil.TempDir("", "kustomize-plugin-tests")
-	if err != nil {
-		x.t.Errorf("failed to make work dir: %v", err)
-	}
-}
-
-func (x *pluginTestEnv) removeWorkDir() {
-	err := os.RemoveAll(x.workDir)
-	if err != nil {
-		x.t.Errorf(
-			"removing work dir: %s %v", x.workDir, err)
-	}
 }
 
 func (x *pluginTestEnv) setEnv() {
 	x.oldXdg, x.wasSet = os.LookupEnv(konfig.KustomizePluginHomeEnv)
-	os.Setenv(konfig.KustomizePluginHomeEnv, x.workDir)
+	os.Setenv(konfig.KustomizePluginHomeEnv, x.pluginRoot)
 }
 
 func (x *pluginTestEnv) resetEnv() {

plugin/builtin/patchtransformer/PatchTransformer_test.go

@@ -102,7 +102,6 @@ patch: "thisIsNotAPatch"
 		if err == nil {
 			t.Fatalf("expected error")
 		}
-		fmt.Print(err)
 		if !strings.Contains(err.Error(),
 			"unable to parse SM or JSON patch from ") {
 			t.Fatalf("unexpected err: %v", err)