Make it possible to build with make --warn-undefined-variables
#98197
Conversation
thockin
added
priority/backlog
k8s-ci-robot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
| @@ -0,0 +1,15 @@ | |||
| load("@io_bazel_rules_go//go:def.bzl", "go_library") | |||
There was a problem hiding this comment.
Are files in .../examples/_examples/... expected?
|
I think the rename somehow got a duplicate nested _examples, can you check that? the other commits look good. |
thockin
force-pushed
the
makefile-unused-variables-watning
branch
2 times, most recently
from
94778a3
to
2595722
Compare
|
Woops, bad rename indeed. Sorry |
|
The dependency failure is legitimate, dropping the underscore makes the examples dir visible to go and sets up a module cycle |
|
I'll look at all the fails, but I am missing the import cycle - where is that? If we do not rename this, the alternatives I see are:
|
thockin
force-pushed
the
makefile-unused-variables-watning
branch
from
2595722
to
5b2d608
Compare
Oh, hrmm... I thought it was a cycle at the module level, but I guess it's not. It does make k8s.io/code-generator depend on diff --git a/staging/src/k8s.io/code-generator/go.mod b/staging/src/k8s.io/code-generator/go.mod
index 52aa67052e8..c0ac68c27c9 100644
--- a/staging/src/k8s.io/code-generator/go.mod
+++ b/staging/src/k8s.io/code-generator/go.mod
@@ -6,20 +6,23 @@ go 1.15
require (
github.com/emicklei/go-restful v2.9.5+incompatible // indirect
+ github.com/go-openapi/spec v0.19.3
github.com/gogo/protobuf v1.3.1
- github.com/google/go-cmp v0.5.2 // indirect
- github.com/json-iterator/go v1.1.10 // indirect
github.com/mailru/easyjson v0.7.0 // indirect
github.com/spf13/pflag v1.0.5
- github.com/stretchr/testify v1.6.1 // indirect
golang.org/x/mod v0.3.0 // indirect
- golang.org/x/net v0.0.0-20201110031124-69a78807bb2b // indirect
- golang.org/x/text v0.3.4 // indirect
golang.org/x/tools v0.0.0-20200616133436-c1934b75d054 // indirect
- golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect
+ k8s.io/api v0.0.0
+ k8s.io/apimachinery v0.0.0
+ k8s.io/client-go v0.0.0
k8s.io/gengo v0.0.0-20201214224949-b6c5ce23f027
k8s.io/klog/v2 v2.4.0
k8s.io/kube-openapi v0.0.0-20201113171705-d219536bb9fd
)
-replace k8s.io/code-generator => ../code-generator
+replace (
+ k8s.io/api => ../api
+ k8s.io/apimachinery => ../apimachinery
+ k8s.io/client-go => ../client-go
+ k8s.io/code-generator => ../code-generator
+)Technically, k8s.io/{api,apimachinery,client-go} probably should depend on k8s.io/code-generator, and their generation scripts should actually live beneath their source trees, but we sort of cheat by having the generation code live in the k8s.io/kubernetes root. |
I mean, it already DOES depend on those, it's just "hidden" in _examples. Do you object to updating the vendoring on this? |
thockin
force-pushed
the
makefile-unused-variables-watning
branch
from
5b2d608
to
b55a883
Compare
k8s-ci-robot
added
the
area/dependency
The examples dir depends on those... but we don't want to expose all consumers of k8s.io/code-generator to dependencies they don't need (specifically k8s.io/api and k8s.io/client-go...)
I would probably isolate the diff --git a/staging/src/k8s.io/code-generator/examples/go.mod b/staging/src/k8s.io/code-generator/examples/go.mod
new file mode 100644
index 00000000000..5a969627e13
--- /dev/null
+++ b/staging/src/k8s.io/code-generator/examples/go.mod
@@ -0,0 +1,5 @@
+// This is a submodule to isolate k8s.io/code-generator from k8s.io/{api,apimachinery,client-go} dependencies in generated code
+
+module k8s.io/code-generator/examples
+
+go 1.15
|
|
+1 to not expanding the dependency graph.
…On Wed, Jan 20, 2021 at 1:38 PM Jordan Liggitt ***@***.***> wrote:
It does make k8s.io/code-generator depend on
k8s.io/{api,apimachinery,client-go}
<http://k8s.io/%7Bapi,apimachinery,client-go%7D>, which will ripple to
anyone consuming k8s.io/code-generator
I mean, it already DOES depend on those, it's just "hidden" in _examples.
The examples dir depends on those... but we don't want to expose all
consumers of k8s.io/code-generator to dependencies they don't need
(specifically k8s.io/api and k8s.io/client-go...)
Do you object to updating the vendoring on this?
I would probably isolate the examples dir with something like this:
diff --git a/staging/src/k8s.io/code-generator/examples/go.mod b/staging/src/k8s.io/code-generator/examples/go.mod
new file mode 100644
index 00000000000..5a969627e13--- /dev/null+++ b/staging/src/k8s.io/code-generator/examples/go.mod@@ -0,0 +1,5 @@+// This is a submodule to isolate k8s.io/code-generator from k8s.io/{api,apimachinery,client-go} <http://k8s.io/%7Bapi,apimachinery,client-go%7D> dependencies in generated code++module k8s.io/code-generator/examples++go 1.15
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#98197 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE6BFSLUQUTXPBRNKYOY63S25EL7ANCNFSM4WJYWJUA>
.
|
Because this comment is in a `define` which is later evaluated, the syntactical `$(eval)` is treated like a variable exapansion. Just change the comment. Driving towards `make --warn-undefined-variables`.
This makes them easier to see and find. Driving towards `make --warn-undefined-variables`.
Driving towards `make --warn-undefined-variables`.
This same dep is expressed a few lines later in the "real" recipe.
Now we can use /dev/null as an argument when running tools manually
Prep to add more such modules
The script would get a 1 return code from grep when there are no files in a directory, after filtering generated filenames. This would cause the script to exit unexpectedly.
The alternative to this would be to special-case code-generator. Since it legit wants codegen, it seems wrong to make it be _examples (which tools should ignore). Make examples an "internal module" so the main go.mod for k8s.io/code-generator does not get too polluted.
This is a no-op now that _examples is renamed.
|
Rebased |
thockin
force-pushed
the
makefile-unused-variables-watning
branch
from
c07026a
to
6fe6941
Compare
k8s-ci-robot
removed
the
needs-rebase
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Currently while zipping modules using the gomod-zip tool, if a repo contains submodules, we include them in the zip archive. However, when we do a go mod download, go skips submodules from the zip archive. This means that the zip file created using gomod-zip is no longer accurate. This also causes the following error when go tries to read the zip file (during `go mod tidy`, etc) and notices submodules in the zip archive: ``` -> unzip /go-workspace/pkg/mod/cache/download/k8s.io/code-generator/@v/v0.0.0-20210126111002-a3ae865d6348.zip: found go.mod file not in module root directory (k8s.io/code-generator@v0.0.0-20210126111002-a3ae865d6348/examples/go.mod) unzip /go-workspace/pkg/mod/cache/download/k8s.io/code-generator/@v/v0.0.0-20210126111002-a3ae865d6348.zip: found go.mod file not in module root directory (k8s.io/code-generator@v0.0.0-20210126111002-a3ae865d6348/examples/go.mod) ``` This specific error occurs because submodules were recently added to code-generator in kubernetes/kubernetes#98197. To fix this issue, this commit ignores submodules while creating the zip archive. The code is derived from https://github.com/golang/go/blob/b373d31c25e58d0b69cff3521b915f0c06fa6ac8/src/cmd/go/internal/modfetch/coderepo.go#L572 but since we already know the package name, this commit removes some complexity from the original go code.
Currently while zipping modules using the gomod-zip tool, if a repo contains submodules, we include them in the zip archive. However, when we do a go mod download, go skips submodules from the zip archive. This means that the zip file created using gomod-zip is no longer accurate. This also causes the following error when go tries to read the zip file (during `go mod tidy`, etc) and notices submodules in the zip archive: kubernetes/kubernetes#56876 (comment) This specific error occurs because submodules were recently added to code-generator in kubernetes/kubernetes#98197. To fix this issue, this commit ignores submodules while creating the zip archive. The code is derived from https://github.com/golang/go/blob/b373d31c25e58d0b69cff3521b915f0c06fa6ac8/src/cmd/go/internal/modfetch/coderepo.go#L572 but since we already know the package name, this commit removes some complexity from the original go code.
|
With a new dependency order (am not a fan), has the publishing order been updated as well? Otherwise, we generate wrong go.mod/sum in the bot I believe cc @nikhita |
The publishing-bot only looks at the root go.mod file today and cannot handle multi-modules. Since the new dependency order is in Right now, the bot will generate this for Are we fine with a wrong published go.mod file for the examples module since it's not meant to be imported? If yes, kubernetes/publishing-bot#244 can help unblock publishing for now. cc @dims |
Currently while zipping modules using the gomod-zip tool, if a repo contains submodules, we include them in the zip archive. However, when we do a go mod download, go skips submodules from the zip archive. This means that the zip file created using gomod-zip is no longer accurate. This also causes the following error when go tries to read the zip file (during `go mod tidy`, etc) and notices submodules in the zip archive: kubernetes/kubernetes#56876 (comment) This specific error occurs because submodules were recently added to code-generator in kubernetes/kubernetes#98197. To fix this issue, this commit ignores submodules while creating the zip archive. The code is derived from https://github.com/golang/go/blob/b373d31c25e58d0b69cff3521b915f0c06fa6ac8/src/cmd/go/internal/modfetch/coderepo.go#L572 but since we already know the package name, this commit removes some complexity from the original go code.
incrementally, I think so... it was in a directory hidden to go before without any dependency information stated |
the goal of the submodule was to avoid touching dependency ordering for modules we intend people to use |
Currently while zipping modules using the gomod-zip tool, if a repo contains submodules, we include them in the zip archive. However, when we do a go mod download, go skips submodules from the zip archive. This means that the zip file created using gomod-zip is no longer accurate. This also causes the following error when go tries to read the zip file (during `go mod tidy`, etc) and notices submodules in the zip archive: kubernetes/kubernetes#56876 (comment) This specific error occurs because submodules were recently added to code-generator in kubernetes/kubernetes#98197. To fix this issue, this commit ignores submodules while creating the zip archive. The code is derived from https://github.com/golang/go/blob/b373d31c25e58d0b69cff3521b915f0c06fa6ac8/src/cmd/go/internal/modfetch/coderepo.go#L572 but since we already know the package name, this commit removes some complexity from the original go code.
/kind cleanup
When you run our build with undefined variable warnings in make, it flags a bunch of errors. In digging some of these are real bugs. After this set of commits, it is possible to rebuild k/k with
--warn-undefined-variablesand get no errors. I specifically said "rebuild". The initial build still flags errors since auto-generated variables have not yet been defined. I'll pursue that separately.I suggest reading this commit-by-commit. The biggest change is renaming code-generator's "_examples" dir, and that can be discussed if need be.