Repository: cilium/ebpf
Pull request #1047 (2023-05-30 14:02:41)

bpf2go: some options take defaults from the environment
Adds `BPF2GO_CC`, `BPF2GO_CFLAGS`, `BPF2GO_STRIP` and `BPF2GO_MAKEBASE`.

The rationale for  `BPF2GO_MAKEBASE`:

This is in addition to existing -makebase option.

When used in go:generate line, the existing option requires a *relative* path from current package directory to the one with the Makefile. (While absolute pathes are accepted, we can't write one since we don't know where a user puts the project in the filesystem.)

Relative paths WILL break as packages move around.  The environment variable, on the other hand, can be set in the Makefile once and doesn't require further maintenance.

See https://github.com/cilium/ebpf/discussions/988#discussioncomment-6037792

mejedi: @lmb That's what I started with (see referenced discussion).

`bpf2go` command invocation quickly gets crowded as evident from the linked tubular example. Won't it be nice if we could take one bit out?

Further, I would argue that using `-makebase ../some/relative/path` is brittle as things will break if packages move around. Therefore `-makebase $VAR` should be preferred. Now this is a little extra effort passed on to the users. If we could bake a little bit of convenience into the tool, won't it be a good thing?

I understand the value in being able to grep for things. However, doesn't `BPF2GO_` prefix suggest that it applies to `bpf2go`?
mejedi: @lmb
> Finally it would be the icing on the cake if you manage to add unit tests.

It's not clear to me how to tackle it best:

  * Should it be a pure unit test focused on argument parsing? I can imagine splitting `run` into pieces (in `cmd/bpf2go/main.go`) and writing assertions on the contents of `bpf2go` structure.
  * Should we rather test bpf2go as whole (as `cmd/bpf2go/main_test.go` does)?
     * Note: there's currently `TestDisableStripping` but no test to verify that stripping works in the first place.
     * Note: hopefully we can test that custom `-cc` works by giving it a path that doesn't exist. Presumably, `os.Exec` error will capture this path. It will work if it is possible to extract `os.Exec` error from the error that `run` returns. Could be fragile though.
mejedi: @lmb 
Added `BPF2GO_CC`, `BPF2GO_CFLAGS`,  and `BPF2GO_STRIP` in addition to `BPF2GO_MAKEBASE` as requested (+unit tests).

I am not very happy with the help text but don't have any ideas how to improve it:

```
Some options take defaults from the environment. Variable name is mentioned
next to the respective option.

Options:

  -cc binary
    	binary used to compile C to BPF (default "clang", $BPF2GO_CC)
  -cflags string
    	flags passed to the compiler, may contain quoted arguments ($BPF2GO_CFLAGS)
  -makebase directory
    	write make compatible depinfo files relative to directory ($BPF2GO_MAKEBASE)
  -no-global-types
    	Skip generating types for map keys and values, etc.

```

Patch:
diff --git a/cmd/bpf2go/main.go b/cmd/bpf2go/main.go
index f05a163..99860d8 100644
--- a/cmd/bpf2go/main.go
+++ b/cmd/bpf2go/main.go
@@ -36,6 +36,9 @@ compiler as two arguments "foo" and "bar baz".
 The program expects GOPACKAGE to be set in the environment, and should be invoked
 via go generate. The generated files are written to the current directory.
 
+Some options take defaults from the environment. Variable name is mentioned
+next to the respective option.
+
 Options:
 
 `
@@ -119,13 +122,17 @@ func newB2G(stdout io.Writer, pkg, outputDir string, args []string) (*bpf2go, er
 	}
 
 	fs := flag.NewFlagSet("bpf2go", flag.ContinueOnError)
-	fs.StringVar(&b2g.cc, "cc", "clang", "`binary` used to compile C to BPF")
-	fs.StringVar(&b2g.strip, "strip", "", "`binary` used to strip DWARF from compiled BPF (default \"llvm-strip\")")
+	fs.StringVar(&b2g.cc, "cc", getEnv("BPF2GO_CC", "clang"),
+		"`binary` used to compile C to BPF ($BPF2GO_CC)")
+	fs.StringVar(&b2g.strip, "strip", getEnv("BPF2GO_STRIP", ""),
+		"`binary` used to strip DWARF from compiled BPF ($BPF2GO_STRIP)")
 	fs.BoolVar(&b2g.disableStripping, "no-strip", false, "disable stripping of DWARF")
-	flagCFlags := fs.String("cflags", "", "flags passed to the compiler, may contain quoted arguments")
+	flagCFlags := fs.String("cflags", getEnv("BPF2GO_CFLAGS", ""),
+		"flags passed to the compiler, may contain quoted arguments ($BPF2GO_CFLAGS)")
 	fs.Var(&b2g.tags, "tags", "Comma-separated list of Go build tags to include in generated files")
 	flagTarget := fs.String("target", "bpfel,bpfeb", "clang target(s) to compile for (comma separated)")
-	fs.StringVar(&b2g.makeBase, "makebase", "", "write make compatible depinfo files relative to `directory`")
+	fs.StringVar(&b2g.makeBase, "makebase", getEnv("BPF2GO_MAKEBASE", ""),
+		"write make compatible depinfo files relative to `directory` ($BPF2GO_MAKEBASE)")
 	fs.Var(&b2g.cTypes, "type", "`Name` of a type to generate a Go declaration for, may be repeated")
 	fs.BoolVar(&b2g.skipGlobalTypes, "no-global-types", false, "Skip generating types for map keys and values, etc.")
 	fs.StringVar(&b2g.outputStem, "output-stem", "", "alternative stem for names of generated files (defaults to ident)")
@@ -261,6 +268,13 @@ func (ct *cTypes) Set(value string) error {
 	return nil
 }
 
+func getEnv(key, defaultVal string) string {
+	if val, ok := os.LookupEnv(key); ok {
+		return val
+	}
+	return defaultVal
+}
+
 func (b2g *bpf2go) convertAll() (err error) {
 	if _, err := os.Stat(b2g.sourceFile); os.IsNotExist(err) {
 		return fmt.Errorf("file %s doesn't exist", b2g.sourceFile)


Test patch:
diff --git a/cmd/bpf2go/main_test.go b/cmd/bpf2go/main_test.go
index 685948d..c890385 100644
--- a/cmd/bpf2go/main_test.go
+++ b/cmd/bpf2go/main_test.go
@@ -294,6 +294,32 @@ func TestParseArgs(t *testing.T) {
 		qt.Assert(t, b2g.makeBase, qt.Equals, basePath)
 	})
 
+	t.Run("makebase from env", func(t *testing.T) {
+		basePath, _ := filepath.Abs("barfoo")
+		args := []string{stem, csource}
+		t.Setenv("BPF2GO_MAKEBASE", basePath)
+		b2g, err := newB2G(&bytes.Buffer{}, pkg, outputDir, args)
+		qt.Assert(t, err, qt.IsNil)
+		qt.Assert(t, b2g.makeBase, qt.Equals, basePath)
+	})
+
+	t.Run("makebase flag overrides env", func(t *testing.T) {
+		basePathFlag, _ := filepath.Abs("barfoo")
+		basePathEnv, _ := filepath.Abs("foobar")
+		args := []string{"-makebase", basePathFlag, stem, csource}
+		t.Setenv("BPF2GO_MAKEBASE", basePathEnv)
+		b2g, err := newB2G(&bytes.Buffer{}, pkg, outputDir, args)
+		qt.Assert(t, err, qt.IsNil)
+		qt.Assert(t, b2g.makeBase, qt.Equals, basePathFlag)
+	})
+
+	t.Run("cc defaults to clang", func(t *testing.T) {
+		args := []string{stem, csource}
+		b2g, err := newB2G(&bytes.Buffer{}, pkg, outputDir, args)
+		qt.Assert(t, err, qt.IsNil)
+		qt.Assert(t, b2g.cc, qt.Equals, "clang")
+	})
+
 	t.Run("cc", func(t *testing.T) {
 		args := []string{"-cc", "barfoo", stem, csource}
 		b2g, err := newB2G(&bytes.Buffer{}, pkg, outputDir, args)
@@ -301,6 +327,29 @@ func TestParseArgs(t *testing.T) {
 		qt.Assert(t, b2g.cc, qt.Equals, "barfoo")
 	})
 
+	t.Run("cc from env", func(t *testing.T) {
+		args := []string{stem, csource}
+		t.Setenv("BPF2GO_CC", "barfoo")
+		b2g, err := newB2G(&bytes.Buffer{}, pkg, outputDir, args)
+		qt.Assert(t, err, qt.IsNil)
+		qt.Assert(t, b2g.cc, qt.Equals, "barfoo")
+	})
+
+	t.Run("cc flag overrides env", func(t *testing.T) {
+		args := []string{"-cc", "barfoo", stem, csource}
+		t.Setenv("BPF2GO_CC", "foobar")
+		b2g, err := newB2G(&bytes.Buffer{}, pkg, outputDir, args)
+		qt.Assert(t, err, qt.IsNil)
+		qt.Assert(t, b2g.cc, qt.Equals, "barfoo")
+	})
+
+	t.Run("strip defaults to llvm-strip", func(t *testing.T) {
+		args := []string{stem, csource}
+		b2g, err := newB2G(&bytes.Buffer{}, pkg, outputDir, args)
+		qt.Assert(t, err, qt.IsNil)
+		qt.Assert(t, b2g.strip, qt.Equals, "llvm-strip")
+	})
+
 	t.Run("strip", func(t *testing.T) {
 		args := []string{"-strip", "barfoo", stem, csource}
 		b2g, err := newB2G(&bytes.Buffer{}, pkg, outputDir, args)
@@ -308,6 +357,22 @@ func TestParseArgs(t *testing.T) {
 		qt.Assert(t, b2g.strip, qt.Equals, "barfoo")
 	})
 
+	t.Run("strip from env", func(t *testing.T) {
+		args := []string{stem, csource}
+		t.Setenv("BPF2GO_STRIP", "barfoo")
+		b2g, err := newB2G(&bytes.Buffer{}, pkg, outputDir, args)
+		qt.Assert(t, err, qt.IsNil)
+		qt.Assert(t, b2g.strip, qt.Equals, "barfoo")
+	})
+
+	t.Run("strip flag overrides env", func(t *testing.T) {
+		args := []string{"-strip", "barfoo", stem, csource}
+		t.Setenv("BPF2GO_STRIP", "foobar")
+		b2g, err := newB2G(&bytes.Buffer{}, pkg, outputDir, args)
+		qt.Assert(t, err, qt.IsNil)
+		qt.Assert(t, b2g.strip, qt.Equals, "barfoo")
+	})
+
 	t.Run("no strip defaults to false", func(t *testing.T) {
 		args := []string{stem, csource}
 		b2g, err := newB2G(&bytes.Buffer{}, pkg, outputDir, args)
@@ -342,6 +407,22 @@ func TestParseArgs(t *testing.T) {
 		qt.Assert(t, err, qt.IsNil)
 		qt.Assert(t, b2g.cFlags, qt.DeepEquals, []string{"x", "y", "z", "u", "v"})
 	})
+
+	t.Run("cflags from env", func(t *testing.T) {
+		args := []string{stem, csource}
+		t.Setenv("BPF2GO_CFLAGS", "x y z")
+		b2g, err := newB2G(&bytes.Buffer{}, pkg, outputDir, args)
+		qt.Assert(t, err, qt.IsNil)
+		qt.Assert(t, b2g.cFlags, qt.DeepEquals, []string{"x", "y", "z"})
+	})
+
+	t.Run("cflags flag overrides env", func(t *testing.T) {
+		args := []string{"-cflags", "u v", stem, csource}
+		t.Setenv("BPF2GO_CFLAGS", "x y z")
+		b2g, err := newB2G(&bytes.Buffer{}, pkg, outputDir, args)
+		qt.Assert(t, err, qt.IsNil)
+		qt.Assert(t, b2g.cFlags, qt.DeepEquals, []string{"u", "v"})
+	})
 }
 
 func clangBin(t *testing.T) string {