Skip to content

✨ Add --load-build-tags flag #1161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 1, 2025
Merged

✨ Add --load-build-tags flag #1161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0236649
Add `--build-tags` flag
joelanford Mar 3, 2025
71cec62
Rename --build-tags to --load-build-tags and add 'ignore_autogenerate…
joelanford Apr 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion cmd/controller-gen/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"strings"

"github.com/spf13/cobra"
"golang.org/x/tools/go/packages"

"sigs.k8s.io/controller-tools/pkg/applyconfiguration"
"sigs.k8s.io/controller-tools/pkg/crd"
Expand Down Expand Up @@ -129,6 +130,7 @@ func main() {
helpLevel := 0
whichLevel := 0
showVersion := false
var buildTags []string

cmd := &cobra.Command{
Use: "controller-gen",
Expand Down Expand Up @@ -173,7 +175,8 @@ func main() {
}

// otherwise, set up the runtime for actually running the generators
Copy link
Member

@sbueringer sbueringer Mar 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loader.LoadRootsWithConfig

	// put our build flags first so that callers can override them
	l.cfg.BuildFlags = append([]string{"-tags", "ignore_autogenerated"}, l.cfg.BuildFlags...)

Is it correct that by adding -tags here we overwrite -tags", "ignore_autogenerated"?

What about using ignore_autogenerated as the default value for our flag?

I think this probably (?) would result in:

  • the same behavior as before if the flag is not set by users
  • allows to add additional tags
  • allows to disable ignore_autogenerated

(not entirely sure, would be good to double check)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would require one to remember to add ignore_autogenerated but it's the most flexible.
Unless we think that ignore_autogenerated is required?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Yes, it appears as if ignore_autogenerated will not be used if tags are provided on the command line.

I implemented a small test project to check behavior. The last -tags flag wins, and all previous tags flags are ignored.

So the behavior of what is in the PR now is:

  • If no tags are passed in the flag, then the last tags flag is "ignore_autogenerated", and that's what is used.
  • If any tags are passed in the flag, "-tags=ignore_autogenerated" is ignored. Only explicit user-defined tags are used.

So I think putting ignore_autogenerated in the default flag value actually has the same outcome, but it is also more explicit, and it makes it easier for our future selves to avoid the question if what happens when -tags is used multiple times.

I can't think of a good reason to force ignore_autogenerated. I think it is better to make that the default but then let a user override that and pass their desired set of tags, which may or may not include ignore_autogenerated.

I'll update the PR to account for this convo.

Copy link
Member Author

@joelanford joelanford Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, there's one problem moving ignore_autogenerated to the flag default. Users of loader.LoadRoots and loader.LoadRootsWithConfig (and any other functions up the call stack that call those functions) will be broken because those functions would suddenly stop including that build tag.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So perhaps we put ignore_autogenerated in the flag default (for visibility/clarity), but then also keep a separate (and overridable) -tags=ignore_autogenerated build flag hardcoded in LoadRootsWithConfig with some heavy comments explaining the situation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me

rt, err := genall.FromOptions(optionsRegistry, rawOpts)
tagsFlag := fmt.Sprintf("-tags=%s", strings.Join(buildTags, ","))
rt, err := genall.FromOptionsWithConfig(&packages.Config{BuildFlags: []string{tagsFlag}}, optionsRegistry, rawOpts)
if err != nil {
return err
}
Expand All @@ -192,6 +195,7 @@ func main() {
cmd.Flags().CountVarP(&whichLevel, "which-markers", "w", "print out all markers available with the requested generators\n(up to -www for the most detailed output, or -wwww for json output)")
cmd.Flags().CountVarP(&helpLevel, "detailed-help", "h", "print out more detailed help\n(up to -hhh for the most detailed output, or -hhhh for json output)")
cmd.Flags().BoolVar(&showVersion, "version", false, "show version")
cmd.Flags().StringSliceVar(&buildTags, "load-build-tags", []string{"ignore_autogenerated"}, "build tags to use when loading Go packages")
cmd.Flags().Bool("help", false, "print out usage and a summary of options")
oldUsage := cmd.UsageFunc()
cmd.SetUsageFunc(func(c *cobra.Command) error {
Expand Down
6 changes: 5 additions & 1 deletion pkg/genall/genall.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,11 @@ func (g GenerationContext) ReadFile(path string) ([]byte, error) {
// ForRoots produces a Runtime to run the given generators against the
// given packages. It outputs to /dev/null by default.
func (g Generators) ForRoots(rootPaths ...string) (*Runtime, error) {
roots, err := loader.LoadRoots(rootPaths...)
return g.ForRootsWithConfig(&packages.Config{}, rootPaths...)
}

func (g Generators) ForRootsWithConfig(cfg *packages.Config, rootPaths ...string) (*Runtime, error) {
roots, err := loader.LoadRootsWithConfig(cfg, rootPaths...)
if err != nil {
return nil, err
}
Expand Down
7 changes: 6 additions & 1 deletion pkg/genall/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"fmt"
"strings"

"golang.org/x/tools/go/packages"
"sigs.k8s.io/controller-tools/pkg/markers"
)

Expand Down Expand Up @@ -74,13 +75,17 @@ func RegistryFromOptions(optionsRegistry *markers.Registry, options []string) (*
// further modified. Not default generators are used if none are specified -- you can check
// the output and rerun for that.
func FromOptions(optionsRegistry *markers.Registry, options []string) (*Runtime, error) {
return FromOptionsWithConfig(&packages.Config{}, optionsRegistry, options)
}

func FromOptionsWithConfig(cfg *packages.Config, optionsRegistry *markers.Registry, options []string) (*Runtime, error) {
protoRt, err := protoFromOptions(optionsRegistry, options)
if err != nil {
return nil, err
}

// make the runtime
genRuntime, err := protoRt.Generators.ForRoots(protoRt.Paths...)
genRuntime, err := protoRt.Generators.ForRootsWithConfig(cfg, protoRt.Paths...)
if err != nil {
return nil, err
}
Expand Down
10 changes: 8 additions & 2 deletions pkg/loader/loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,8 +374,14 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) ([]*Package, err
if l.cfg.Fset == nil {
l.cfg.Fset = token.NewFileSet()
}
// put our build flags first so that callers can override them
l.cfg.BuildFlags = append([]string{"-tags", "ignore_autogenerated"}, l.cfg.BuildFlags...)

// put our build flags first so that callers can override them.
//
// NOTE: if callers provide their own `-tags` flag, then our hardcoded `ignore_autogenerated` tag
// will be ignored. Callers that provide custom tags MUST include `ignore_autogenerated` in their
// custom tag set if they want that tag to remain active. Users can explicitly pass custom build
// flags with `-tags=""` to disable use of the default `ignore_autogenerated` tag.
l.cfg.BuildFlags = append([]string{"-tags=ignore_autogenerated"}, l.cfg.BuildFlags...)

// Visit the import graphs of the loaded, root packages. If an imported
// package refers to another loaded, root package, then replace the
Expand Down
Loading