Don't identity-alias imports

[brandond]

Fixes stuff like context "context" and http "net/http".

Ref:

• Performance: Use FormatOnly on imports #277 (comment)

[k8s-ci-robot]

Welcome @brandond!

It looks like this is your first PR to kubernetes/gengo 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/gengo has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @brandond. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[brandond]

@thockin any chance of getting this upstreamed or should I just close it?

[thockin]

I'm not sure this is sufficient. It's rare but sometimes the package name and the leaf of the path are not the same. Looking at this code, I doubt we handle it properly in the absence of an alias like this.

This is one area where my big refactoring last year dared not go - the importer logic is twisty. I figured it's generated code, so who cares, really. I am not against fixing it, but I think that a change like this will explode in the above case.

@jpbetz

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[brandond]

would still love to get this in, just from a housekeeping perspective

[thockin]

Did you try to patch this into k/k and see what happens?

[thockin]

I patched it in and the diff is huge but looks sane, e.g.:

diff --git a/pkg/apis/networking/v1/zz_generated.conversion.go b/pkg/apis/networking/v1/zz_generated.conversion.go
index 3e63e44abd9..ae5211ddff8 100644
--- a/pkg/apis/networking/v1/zz_generated.conversion.go
+++ b/pkg/apis/networking/v1/zz_generated.conversion.go
@@ -22,16 +22,16 @@ limitations under the License.
 package v1
 
 import (
-       unsafe "unsafe"
+       "unsafe"
 
        corev1 "k8s.io/api/core/v1"
        networkingv1 "k8s.io/api/networking/v1"
        metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-       conversion "k8s.io/apimachinery/pkg/conversion"
-       runtime "k8s.io/apimachinery/pkg/runtime"
-       intstr "k8s.io/apimachinery/pkg/util/intstr"
-       core "k8s.io/kubernetes/pkg/apis/core"
-       networking "k8s.io/kubernetes/pkg/apis/networking"
+       "k8s.io/apimachinery/pkg/conversion"
+       "k8s.io/apimachinery/pkg/runtime"
+       "k8s.io/apimachinery/pkg/util/intstr"
+       "k8s.io/kubernetes/pkg/apis/core"
+       "k8s.io/kubernetes/pkg/apis/networking"
 )
 
 func init() {

The questions are a) whether it compiles (can't test here, battery dying); and b) whether we like this better or not, or don't care.

[jpbetz]

The questions are a) whether it compiles (can't test here, battery dying); and b) whether we like this better or not, or don't care.

+1, I think for this case it's worth opening a I draft PR against github.com/kubernetes/kuberetes that uses hack/pin-dependency.sh to pull in a dev version of this change. I'd like to see such a PR pass all presubmits before we merge into gengo. Should be pretty easy given that hack/pin-dependency.sh supports stuff like hack/pin-dependency.sh github.com/docker/docker=github.com/johndoe/docker my-experimental-branch now.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: brandond
Once this PR has been reviewed and has the lgtm label, please assign sttts for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[brandond]

Opened kubernetes/kubernetes#132661 @jpbetz @thockin

[thockin]

The goal is to get a CI run that passes k/k (or fails only for reasons explained by the vedor hackery).

Unfortunately, that PR is huge and already needs a rebase. :(

[brandond]

am I running the correct commands to touch everything that would need touching?

[thockin]

I could not shake the feeling that this was broken for cases where the dirname and pkgname did not match, so I hacked (seriously ugly) a test case. Yeah, it's broken.

diff --git a/v2/examples/pointuh/main.go b/v2/examples/pointuh/main.go
index a469f38..5d5fe40 100644
--- a/v2/examples/pointuh/main.go
+++ b/v2/examples/pointuh/main.go
@@ -163,6 +163,7 @@ func getTargets(c *generator.Context, args *Args) []generator.Target {
 type pointuhGenerator struct {
 	generator.GoGenerator
 	myPackage *types.Package
+	imports   namer.ImportTracker
 }
 
 func newPointuhGenerator(outputFilename string, pkg *types.Package) generator.Generator {
@@ -171,6 +172,7 @@ func newPointuhGenerator(outputFilename string, pkg *types.Package) generator.Ge
 			OutputFilename: outputFilename,
 		},
 		myPackage: pkg,
+		imports:   generator.NewImportTrackerForPackage(pkg.Name),
 	}
 }
 
@@ -181,6 +183,7 @@ 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),
+		"raw":      namer.NewRawNamer("", g.imports),
 	}
 }
 
@@ -203,13 +206,33 @@ func (g *pointuhGenerator) GenerateType(c *generator.Context, t *types.Type, w i
 	return sw.Error()
 }
 
+func (g *pointuhGenerator) Imports(_ *generator.Context) (imports []string) {
+	var importLines []string
+	for _, singleImport := range g.imports.ImportLines() {
+		importLines = append(importLines, singleImport)
+	}
+	return importLines
+}
+
+var done = false
+
 func emitUnmodifiedType(t *types.Type, sw *generator.SnippetWriter) {
 	if t.Kind == types.DeclarationOf || t.Kind == types.Interface {
 		return
 	}
 
 	args := argsFromType(t)
+	args["foo"] = &types.Type{
+		Name: types.Name{
+			Name:    "Foo",
+			Package: "k8s.io/gengo/v2/foodir",
+		},
+	}
 	sw.Do("// $.type|localraw$ is an autogenerated clone of $.type|raw$\n", args)
+	if !done {
+		sw.Do("type foo $.foo|raw$ \n", args)
+		done = true
+	}
 	sw.Do("type $.type|localraw$ ", args)
 	for {
 		if t.Kind != types.Pointer {
diff --git a/v2/foodir/foo.go b/v2/foodir/foo.go
new file mode 100644
index 0000000..76c9182
--- /dev/null
+++ b/v2/foodir/foo.go
@@ -0,0 +1,3 @@
+package foopkg
+
+type Foo struct{}
diff --git a/v2/generator/import_tracker.go b/v2/generator/import_tracker.go
index 22393e4..1931636 100644
--- a/v2/generator/import_tracker.go
+++ b/v2/generator/import_tracker.go
@@ -47,7 +47,13 @@ func NewImportTrackerForPackage(local string, typesToAdd ...*types.Type) *namer.
 	tracker := namer.NewDefaultImportTracker(types.Name{Package: local})
 	tracker.IsInvalidType = func(*types.Type) bool { return false }
 	tracker.LocalName = func(name types.Name) string { return goTrackerLocalName(&tracker, local, name) }
-	tracker.PrintImport = func(path, name string) string { return name + " \"" + path + "\"" }
+	tracker.PrintImport = func(path, name string) string {
+		dirs := strings.Split(path, namer.GoSeparator)
+		if len(dirs) > 0 && name == dirs[len(dirs)-1] {
+			return "\"" + path + "\""
+		}
+		return name + " \"" + path + "\""
+	}
 
 	tracker.AddTypes(typesToAdd...)
 	return &tracker
@@ -61,13 +67,13 @@ func goTrackerLocalName(tracker namer.ImportTracker, localPkg string, t types.Na
 	path := t.Package
 
 	// Using backslashes in package names causes gengo to produce Go code which
-	// will not compile with the gc compiler. See the comment on GoSeperator.
+	// will not compile with the gc compiler. See the comment on GoSeparator.
 	if strings.ContainsRune(path, '\\') {
 		klog.Warningf("Warning: backslash used in import path '%v', this is unsupported.\n", path)
 	}
 	localLeaf := filepath.Base(localPkg)
 
-	dirs := strings.Split(path, namer.GoSeperator)
+	dirs := strings.Split(path, namer.GoSeparator)
 	for n := len(dirs) - 1; n >= 0; n-- {
 		// follow kube convention of not having anything between directory names
 		name := strings.Join(dirs[n:], "")
diff --git a/v2/namer/namer.go b/v2/namer/namer.go
index bae2ee9..2202f8e 100644
--- a/v2/namer/namer.go
+++ b/v2/namer/namer.go
@@ -26,14 +26,17 @@ import (
 )
 
 const (
-	// GoSeperator is used to split go import paths.
+	// GoSeparator is used to split go import paths.
 	// Forward slash is used instead of filepath.Seperator because it is the
 	// only universally-accepted path delimiter and the only delimiter not
 	// potentially forbidden by Go compilers. (In particular gc does not allow
 	// the use of backslashes in import paths.)
 	// See https://golang.org/ref/spec#Import_declarations.
 	// See also https://github.com/kubernetes/gengo/issues/83#issuecomment-367040772.
-	GoSeperator = "/"
+	GoSeparator = "/"
+	// GoSeperator is a typo for GoSeparator.
+	// Deprecated: use GoSeparator instead.
+	GoSeperator = GoSeparator
 )
 
 // Returns whether a name is a private Go name.
@@ -200,7 +203,7 @@ var (
 
 // filters out unwanted directory names and sanitizes remaining names.
 func (ns *NameStrategy) filterDirs(path string) []string {
-	allDirs := strings.Split(path, GoSeperator)
+	allDirs := strings.Split(path, GoSeparator)
 	dirs := make([]string, 0, len(allDirs))
 	for _, p := range allDirs {
 		if ns.IgnoreWords == nil || !ns.IgnoreWords[p] {

I fixed a typo in the process, sorry for the noise.

When I run this against the simple case (go run ./examples/pointuh/ --output-dir ./footest --output-pkg example.com/footest ./examples/pointuh/testdata/simple/), I get:

import (
        "k8s.io/gengo/v2/foodir"
)

and

type foo foodir.Foo

That's clearly not correct -- the package's name is "foopkg". We either need the namer to emit foopkg or the importline to alias back to foodir (even though it seems redundant).

We could maybe put a link to the packages.Package in each Type, so the import printer could see that the name and dir don't match, and alias it back to the dir.

Or we could teach namers how to look up the packages own name. This is almost certainly more correct, but probably STILL ends up putting a link to the packages.Package in each Type. There are so many layers of indirection in this area that it's hard to tell from reading what would be needed or what would be impacted if we changed it (and at what point in the lifecycle these things really happen).

Or we could just not. Maybe the best fix here is just a comment documenting this as "load-bearing ugliness"?

[brandond]

Ugh, I always forget that package names don't actually have to match paths.

I'm willing to put in a bit more poking to fix it, as we are still working through a backlog of projects to update to more recent versions of kubernetes and gengo, and every time we do so someone says "why is it adding all these weird aliases to the imports"¹. At least now I have this issue to link to, I guess - but it would save a lot of person-hours if it just didn't do this in the first place.

Footnotes

1. https://github.com/k3s-io/helm-controller/pull/281#discussion_r2183249361 ↩

[thockin]

If you want to poke at it, you at least have a (hacktastic) test. We should put a real test in for this, of course.

I would love to see an overhaul of Namers and Imports. I didn't tackle it during the v1->v2 transition because it was already just SO MUCH change. I don't know this part of the code that well, and I don't know what the resault should look like, so I don't even have a whole lot of guidance to offer.

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle rotten
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[brandond]

/remove-lifecycle rotten

Still working on this