bpf2go: some options take defaults from the environment

[mejedi]

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 #988 (reply in thread)

[lmb]

Originally I didn't add this because go:generate automatically makes environment vars available to the command line. Something like

//go:generate bpf2go -makebase "$BPF2GO_MAKEBASE" ...

allows you to set the makebase once, in the main Makefile. See tubular for an example. This has the benefit that you can easily grep a project and figure out where the variable is used. Would this solve your problem as well?

[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?

[lmb]

Yeah adding that convenience is fine for me. I agree that -makebase should always be used with an env variable anways. I have a couple of requests though. If we're adding env support, let's add some more:

• BPF2GO_CC
• BPF2GO_STRIP
• BPF2GO_CFLAGS
• Any others stand out?

Instead of cluttering individual flags help texts, why not update helpText and mentioned that some flags may default to environment variables and only mention the vars in the per-flag text? Something like write make compatible depinfo files relative to directory ($BPF2GO_MAKEBASE).

Can you also document that flags always take precedence over env variables?

Finally it would be the icing on the cake if you manage to add unit tests.

[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.

[lmb]

I can imagine splitting run into pieces (in cmd/bpf2go/main.go) and writing assertions on the contents of bpf2go structure.

Yeah that would be great. Seems like it should be possible to split just above here?

ebpf/cmd/bpf2go/main.go

Line 195 in 940221e

for target, arches := range targets {

[mejedi]

@lmb

I can imagine splitting run into pieces (in cmd/bpf2go/main.go) and writing assertions on the contents of bpf2go structure.

Yeah that would be great. [...]

#1054

[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.

[mejedi]

@lmb Thank you for the review. I've done requested changes.

[lmb]

Looks great, thanks for working on this!