Spawn gengo/v2 (KEP 4402) #259

thockin · 2024-01-28T04:16:37Z

First: This PR can be broken up!

This clones gengo into gengo/v2 and overhauls it to use Go's packages.Load() instead of DIY. It includes a number of refactorings and simplifcations, too, which really warrant being "v2".

The "examples" (which were actually prod tools) get moved to k/k and replaced with simpler examples.

The commits in this PR were adapted from kubernetes/kubernetes#122624

I have tried to make sure test coverage did not drop and that tests pass at every commit.

v2/generator/execute.go

v2/parser/parse.go

v2/generator/execute.go

skitt · 2024-01-29T10:52:14Z

v2/args/args.go

@@ -137,7 +137,7 @@ func (g *GeneratorArgs) NewBuilder() (*parser.Builder, error) {
 // Execute implements main().
 // If you don't need any non-default behavior, use as:
 // args.Default().Execute(...)
-func (g *GeneratorArgs) Execute(nameSystems namer.NameSystems, defaultSystem string, pkgs func(*generator.Context, *GeneratorArgs) []generator.Package) error {
+func (g *GeneratorArgs) Execute(nameSystems namer.NameSystems, defaultSystem string, getTargets func(*generator.Context, *GeneratorArgs) []generator.Target) error {


If the goal is to distinguish input and output packages, how about making that explicit in the type names? InputPackage / OutputPackage, or InPackage / OutPackage, or InputPkg etc.

I'm unclear what you are proposing? Can you offer a diff or be more precise?

I was reviewing this in the context of da029b0 — the commit message says

Having two types names "Package" was super confusing to read. I don't love "Target" but at least it implies "this is the output" rather than the input.

Since the use of Package is ambiguous, it seems to me that a possible solution would be to disambiguate both uses cases instead of using a not-great replacement for the output scenario; InputPackage / OutputPackage for example.

Oh, I see. I was shooting for something shortish rather than renaming types.Package at this time. In truth, I rather hate the way gengo has its own Package type with (IMO) dangerous lazy-load semantics and how it interposes its own Type and Kind types with different semantics from Go. I'd like to see use eliminate all of that, but it wasn't necessary to this PR and I really want to get workspaces support into k/k (which is blocked on this :)

So - we could totally do a followup to rename things more :)

That’s fine by me!

thockin · 2024-01-29T20:40:14Z

Fixed minor feedback (thanks!)

jpbetz · 2024-01-30T23:38:24Z

v2/comments.go

+		}
+	}
+	return out
+}


@alexzielenski Just wanted you to be aware this is here. I don't know if we'll need to follow up later to somehow reconcile this with kubernetes/kube-openapi#446

I'm not sure we will since a key difference between the two is the kube-openapi version does not allow implicit arrays by duplicating markers which I dont think would be backwards compatible if we merged the two (unless it was a parser optoin or something, but at that rate they could be kept separate I think)

It would be good to have one convention that applied to all tools. FWIW I didn't change this logic, just copied it over. :)

jpbetz · 2024-01-31T00:26:15Z

v2/hack/verify-examples.sh

@@ -0,0 +1,54 @@
+#!/usr/bin/env bash
+
+# hack/verify-examples.sh


Just thinking ahead for a moment..

We should tool our testing infra to run v2/hack/verify-examples.sh? We could run it from the root hack/verify-examples.sh but that running it separately will probably result in clearer error reporting.

Good point! Should we move this to github actions or keep the config in https://github.com/kubernetes/test-infra/tree/a446b9e441a3b25166a8084f32f5ae16ac76a208/config/jobs/kubernetes/gengo ?

v2/README.md

v2/generator/simple_package.go

alexzielenski · 2024-01-31T01:19:22Z

v2/comments.go

+		}
+	}
+	return out
+}


I'm not sure we will since a key difference between the two is the kube-openapi version does not allow implicit arrays by duplicating markers which I dont think would be backwards compatible if we merged the two (unless it was a parser optoin or something, but at that rate they could be kept separate I think)

v2/README.md

v2/generator/import_tracker.go

v2/examples/kilroy/main.go

alexzielenski · 2024-01-31T01:31:28Z

v2/examples/pointuh/main.go

+func (g *pointuhGenerator) Namers(*generator.Context) namer.NameSystems {
+	return namer.NameSystems{
+		// This elides package names when the name is in "this" package.
+		"localraw": namer.NewRawNamer(g.myPackage.Path, nil),


I've seen this pattern in a few examples. Is this something that could make a sensible default? I feel like the local-aware namer and per-target import trackers are something consumers of this library wouldnt mind being plumbed for them?

I do think we could simplify how namers work more - I didn't cover it in this PR because it was already too big.

thockin · 2024-01-31T18:17:07Z

pushed with some comments addressed

thockin · 2024-02-16T23:36:47Z

OK. KEPs are done. I know this will take time, so I'm sending my first ping for review :)

alexzielenski

lgtm

I think we all agree on general approach of introducing gengo/v2. I can't find anything blocking on the few passes I've made. Since this is net-new change, with working prototypes and no comments for a while, should we merge this and fix any issues that come up as we use it?

v2/examples/deepcopy-gen/generators/deepcopy.go

v2/hack/verify-examples.sh

liggitt · 2024-02-23T06:50:52Z

v2/examples/defaulter-gen/generators/defaulter.go

+				// this is a relative dir, which will not work under gomodules.
+				// join with the local package path, but warn
+				klog.Warningf("relative path %s=%s will not work under gomodule mode; use full package path (as used by 'import') instead", inputTagName, inputPath)
+				inputPath = filepath.Join(pkg.Path, inputTags[0])


~~is filepath correct if we're constructing a package path? should we be using path.Join since import paths always use /?~~

edit: I see this got moved to k/k, will ask there

Interesting distinction I didn't consider. It's become habit to always use filepath. I'm not sure these tools run on Windows to know if this breaks or if path is better.

liggitt · 2024-02-23T06:52:18Z

v2/examples/defaulter-gen/generators/defaulter.go

+	if customArgs, ok := arguments.CustomArgs.(*CustomArgs); ok {
+		for _, pkg := range customArgs.ExtraPeerDirs {
+			if i := strings.Index(pkg, "/vendor/"); i != -1 {
+				pkg = pkg[i+len("/vendor/"):]


~~this means they're giving file paths, not import paths, right? should we warn about this now as well if it is no longer necessary?~~

edit: I see this got moved to k/k, will ask there

To hit this they would have to have said ./vendor/example.com/project and I think that packages.Load would still DTRT.

README.md

v2/examples/pointuh/main.go

v2/args/args.go

v2/examples/deepcopy-gen/generators/deepcopy.go

v2/parser/parse.go

v2/generator/simple_target.go

Why? - It's not a great "example". - It doesn't really need gengo at all (in fact, it is retooled in k/k PR to not use gengo) - k8s.io/code-generator has an alternate `main()` anyway

Support for tests in x/tools/go/packages is pretty hostile to gengo, and nobody uses it any more. This also allows us to delete the "incoming imports" logic.

Each tool has its own Args type, now - there are no "standard" arguments.

Now callers can just say `gengo.Execute`

It served its purpose and is now just noise.

thockin · 2024-02-24T23:47:14Z

Pushed with comments addressed (mostly new commits, but some fixed in-line).

https://github.com/kubernetes/gengo/compare/071b86b8ca2508407c999f2a488d017915f2c707..e7d5fc5e02f813189a58e771fa753b85f21b3de9

liggitt · 2024-02-24T23:53:18Z

/lgtm
/hold until this is pulled into the k/k PR and we're happy with the two generators moved there on top of this latest content

k8s-ci-robot · 2024-02-24T23:53:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

thockin · 2024-02-25T00:07:06Z

It's easier for me to merge this and then fix any ripple into the k/k and openapi PRs

thockin assigned alexzielenski, jpbetz and liggitt Jan 28, 2024

k8s-ci-robot requested review from alexzielenski and jpbetz January 28, 2024 04:16

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 28, 2024

thockin force-pushed the master branch from d68a6f2 to 97f7adc Compare January 28, 2024 04:37

This was referenced Jan 28, 2024

Deep copy gen generates invalid code if struct has an _ field #133

Closed

Support parsing directives in comment of methods, structs, struct members and package doc #257

Closed

Go workspaces for k/k and staging kubernetes/kubernetes#122624

Closed

skitt reviewed Jan 29, 2024

View reviewed changes

thockin force-pushed the master branch from 49a0fdc to e8a982a Compare January 29, 2024 20:40

jpbetz reviewed Jan 31, 2024

View reviewed changes

alexzielenski reviewed Jan 31, 2024

View reviewed changes

thockin force-pushed the master branch from e8a982a to e37a30e Compare January 31, 2024 18:16

thockin force-pushed the master branch from e37a30e to 071b86b Compare January 31, 2024 18:19

alexzielenski reviewed Feb 22, 2024

View reviewed changes

v2/examples/deepcopy-gen/generators/deepcopy.go Outdated Show resolved Hide resolved

liggitt reviewed Feb 23, 2024

View reviewed changes

thockin added 20 commits February 24, 2024 15:45

v2: Get rid of import-boss (moved to k/k)

eb27a87

Why? - It's not a great "example". - It doesn't really need gengo at all (in fact, it is retooled in k/k PR to not use gengo) - k8s.io/code-generator has an alternate `main()` anyway

v2: Drop test-files support

ea6ce31

Support for tests in x/tools/go/packages is pretty hostile to gengo, and nobody uses it any more. This also allows us to delete the "incoming imports" logic.

v2: Get rid of InputDirs flag - just use args

13c3e12

v2: Get rid of deepcopy-gen (moved to k/k)

8efa95a

v2: Get rid of defaulter-gen (moved to k/k)

b08ffa4

v2: Get rid of CustomArgs entirely

ccc4c0d

Each tool has its own Args type, now - there are no "standard" arguments.

v2: Get rid of the 'args' pkg

3446e13

Now callers can just say `gengo.Execute`

v2: Remove now-unused FlattenMembers

8679c27

v2: Move utils from gengo/v2/types -> gengo/v2

d5e4a35

v2: Rename 'tc' import to 'gotypes'

bb5d06d

v2: Drop importPathString type

e08acb0

It served its purpose and is now just noise.

v2: Rename Builder -> Parser

4d1b815

v2: Rename SourcePath -> Dir

e246f57

v2: No need to check nil iterating Inputs

d43ccb5

v2: Only log it the first time a package is added

780d761

v2: Adapt PR 244 for v2

008dc98

Add a top-level 'make test' for v0 and v2

f79cce5

v2: verify script chdir to v2 root

9d8f49d

v2: s/Golang/Go/

483edb0

v2: Pass BuildTags in an Options struct

e7d5fc5

thockin force-pushed the master branch from 071b86b to e7d5fc5 Compare February 24, 2024 23:46

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 24, 2024

thockin removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 25, 2024

k8s-ci-robot merged commit 471f57d into kubernetes:master Feb 25, 2024
4 checks passed

thockin mentioned this pull request Feb 25, 2024

Go workspaces for k/k kubernetes/enhancements#4402

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spawn gengo/v2 (KEP 4402) #259

Spawn gengo/v2 (KEP 4402) #259

thockin commented Jan 28, 2024

skitt Jan 29, 2024

thockin Jan 29, 2024

skitt Jan 30, 2024

thockin Jan 30, 2024

skitt Jan 30, 2024

thockin commented Jan 29, 2024

jpbetz Jan 30, 2024

alexzielenski Jan 31, 2024 •

edited

Loading

thockin Jan 31, 2024

jpbetz Jan 31, 2024

thockin Jan 31, 2024 •

edited

Loading

alexzielenski Jan 31, 2024 •

edited

Loading

alexzielenski Jan 31, 2024

thockin Jan 31, 2024

thockin commented Jan 31, 2024

thockin commented Feb 16, 2024

alexzielenski left a comment

liggitt Feb 23, 2024

thockin Feb 24, 2024

liggitt Feb 23, 2024

thockin Feb 24, 2024

thockin commented Feb 24, 2024 •

edited

Loading

liggitt commented Feb 24, 2024

k8s-ci-robot commented Feb 24, 2024

thockin commented Feb 25, 2024

		@@ -0,0 +1,54 @@
		#!/usr/bin/env bash

		# hack/verify-examples.sh

Spawn gengo/v2 (KEP 4402) #259

Spawn gengo/v2 (KEP 4402) #259

Conversation

thockin commented Jan 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Jan 29, 2024

Choose a reason for hiding this comment

alexzielenski Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

alexzielenski Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Jan 31, 2024

thockin commented Feb 16, 2024

alexzielenski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Feb 24, 2024 • edited Loading

liggitt commented Feb 24, 2024

k8s-ci-robot commented Feb 24, 2024

thockin commented Feb 25, 2024

alexzielenski Jan 31, 2024 •

edited

Loading

thockin Jan 31, 2024 •

edited

Loading

alexzielenski Jan 31, 2024 •

edited

Loading

thockin commented Feb 24, 2024 •

edited

Loading