Skip to content
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.

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

No GOPATH environment variable breaks code generators #359

Closed
mheese opened this issue Aug 17, 2018 · 16 comments
Closed

No GOPATH environment variable breaks code generators #359

mheese opened this issue Aug 17, 2018 · 16 comments
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@mheese
Copy link

mheese commented Aug 17, 2018

Problem

If one uses the default gopath at $HOME/go, and therefore does not set a GOPATH environment variable, the deepcopy-gen will save its output files into the wrong destination path.

This is the output if one just tries to follow the quickstart without a defined GOPATH environment variable:

go generate ./pkg/... ./cmd/...
go fmt ./pkg/... ./cmd/...
go vet ./pkg/... ./cmd/...
# ships/pkg/apis/ships/pkg/apis/ships/v1beta1
pkg/apis/ships/pkg/apis/ships/v1beta1/zz_generated.deepcopy.go:27:11: undefined: Sloop
make: *** [Makefile:38: vet] Error 2

As one can see the output gets saved to pkg/apis/ships/pkg/apis/ships, so the relative output base is wrong.

The deepcopygen command line is defined in the pkg/apis/apis.go like this:

// Generate deepcopy for apis
//go:generate go run ../../vendor/k8s.io/code-generator/cmd/deepcopy-gen/main.go -O zz_generated.deepcopy -i ./... -h ../../hack/boilerplate.go.txt

It is not setting the output base on the command line, which works like this according to the help:

-o, --output-base string               Output base; defaults to $GOPATH/src/ or ./ if $GOPATH is not set. (default "./")

As there is no GOPATH set, the default is the current working directory, and this is why there is the duplicate pkg/apis/ships path in the destination.

Workaround:

Simply set your GOPATH environment variable like:

export GOPATH="$HOME/go"

Possible Solutions:

  1. simply document the workaround that the GOPATH environment variable must be set
  2. maybe be specific in the go:generate command line and add a -o
  3. change the default behaviour for deepcopy-gen ? I don't think this is a good idea though, it's pretty clear how it should work
@droot
Copy link
Contributor

droot commented Aug 17, 2018

I like the option 2 that you suggested. Do you mind sharing the value for parameter for -o that works for you ?

If you are interested, this is where additional argument can be supplied to fix it. https://github.com/kubernetes-sigs/controller-tools/blob/master/pkg/scaffold/manager/apis.go#L41

@droot droot added help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. labels Aug 17, 2018
fraenkel added a commit to fraenkel/kubebuilder that referenced this issue Aug 18, 2018
When there is no GOPATH, deepcopy-gen must set the output base. Use the
build package to determine what the appropriate GOPATH is regardless.

Fixes kubernetes-sigs#359
fraenkel added a commit to fraenkel/kubebuilder that referenced this issue Aug 18, 2018
When there is no GOPATH, deepcopy-gen must set the output base. Use the
build package to determine what the appropriate GOPATH is regardless.

Fixes kubernetes-sigs#359
@fraenkel
Copy link
Contributor

fraenkel commented Aug 19, 2018

So the problem is that there are other go generate stanzas that are outside the control of kubebuiler.
When I tried without a GOPATH, I get the constant failure as shown by the CI on my closed PR.
The issue is in pkg/apis/apis.go, it has a go generate that will not work.

So I am running without a GOPATH, but I don't get the failures above.
I have forked controller-tools and updated the Gopkg.toml to point there, but I never see the generated code actually add the -o option that I added.

@mheese
Copy link
Author

mheese commented Aug 20, 2018

@fraenkel, I don't understand from your explanation why you don't experience the problem without a GOPATH, however, there are also a couple of other things that I don't understand:

  1. using build.Default.GOPATH sounds like a good idea to me, and would only break if the HOME environment variable is not set - which is unlikely. However, what about using relative paths instead?

  2. kubebuilder ships with a build of deepcopy-gen, so why are we using a go run ... command?

  3. I'm also trying to make the controller-tools produce an updated apis.go package, but yes, it does not seem to get used. However, I could not find any other code places where this is actually going to get used.

@fraenkel
Copy link
Contributor

Once I realized my mistake, I reworked the PR to only add the base when the GOPATH was missing. At the time I also noticed that the generate line wasn't changing. So I decided to replicate your original issue first so I could see where things were really breaking. But that too proved to be a problem. I was running with no GOPATH but not hitting any issue.
Like you I found a lot of little quirks, like the tools need to live in /usr/local and not on the PATH for certain commands.
Anyhow, I haven't gone back to replicate the error which I need to do first or else I don't know if and when I am fixing the issue.

@mheese
Copy link
Author

mheese commented Aug 21, 2018

hmm... interesting

@fraenkel, I wonder what the difference is in our setups, I have the following things installed:

  • go 1.10.3 at /usr/local/go
  • kubebuilder 1.0.1 at /usr/local/kubebuilder
  • my project is located $HOME/go/src/ships
  • I run the commands with the current working directory at $HOME/go/src/ships
  • my PATH has the following order (among other things) ...:/home/mheese/go/bin:/usr/local/go/bin:/usr/local/kubebuilder/bin:...

And I'm running Arch Linux if that makes any difference.

@fraenkel
Copy link
Contributor

Just tried it again.
Started with the code from master, wiped my /tmp/kubebuilder, rebuilt kubebuilder.
Untarred the dist tar.gz to /usr/local.
Unset my GOPATH
Created a directory under ~/go/src/ships. Changed my directory.
Invoked kubebuilder init ships, said yes to dep ensure.
Invoked kubebuilder create api --group ships --kind Sloop --version v1beta1, created the resource and controller.
Everything completed.

riting scaffold for you to edit...
pkg/apis/ships/v1beta1/sloop_types.go
pkg/apis/ships/v1beta1/sloop_types_test.go
pkg/controller/sloop/sloop_controller.go
pkg/controller/sloop/sloop_controller_test.go
Running make...
go generate ./pkg/... ./cmd/...
go fmt ./pkg/... ./cmd/...
go vet ./pkg/... ./cmd/...
go run vendor/sigs.k8s.io/controller-tools/cmd/controller-gen/main.go all
CRD manifests generated under '/home/fraenkel/go/src/ships/config/crds' 
....

Using Go 1.10.3 on Ubuntu 18.04.

@mheese
Copy link
Author

mheese commented Aug 31, 2018

@fraenkel , I reproduced the issue on Ubuntu 18.04. Here is a Vagrantfile (NOTE: I'm using the libvirt provider, replace for whatever works for you):

Vagrant.configure("2") do |config|
  config.vm.box = "generic/ubuntu1804"
  config.vm.provider :libvirt do |libvirt|
    libvirt.cpus = 4
    libvirt.memory = "6192"
    libvirt.graphics_type = "spice"
  end
  config.vm.provision "shell", inline: <<-SHELL
    cd /root
    apt-get install -y wget make
    wget -q https://dl.google.com/go/go1.10.4.linux-amd64.tar.gz
    tar -C /usr/local -xzf go1.10.4.linux-amd64.tar.gz
    export PATH=/root/go/bin:/usr/local/go/bin:$PATH
    go get -u github.com/golang/dep/cmd/dep
    wget -q https://github.com/kubernetes-sigs/kubebuilder/releases/download/v1.0.2/kubebuilder_1.0.2_linux_amd64.tar.gz
    tar -C /usr/local -xzf kubebuilder_1.0.2_linux_amd64.tar.gz
    cd /usr/local && ln -s kubebuilder_1.0.2_linux_amd64 kubebuilder && cd /root
    export PATH=/usr/local/kubebuilder/bin:$PATH
    mkdir -p /root/go/src/ships
    cd /root/go/src/ships
    yes | kubebuilder init --domain k8s.io --license apache2 --owner "The Kubernetes Authors"
    yes | kubebuilder create api --group ships --version v1beta1 --kind Sloop
    make
  SHELL
end

And here is the last part of the provisioning output:

...
...
...
    default: Run `dep ensure` to fetch dependencies (Recommended) [y/n]?
    default: dep ensure
    default: Running make...
    default: make
    default: go generate ./pkg/... ./cmd/...
    default: go fmt ./pkg/... ./cmd/...
    default: go vet ./pkg/... ./cmd/...
    default: go run vendor/sigs.k8s.io/controller-tools/cmd/controller-gen/main.go all
    default: CRD manifests generated under '/root/go/src/ships/config/crds' 
    default: RBAC manifests generated under '/root/go/src/ships/config/rbac' 
    default: go test ./pkg/... ./cmd/... -coverprofile cover.out
    default: ?   	ships/pkg/apis	[no test files]
    default: ?   	ships/pkg/controller	[no test files]
    default: ?   	ships/cmd/manager	[no test files]
    default: go build -o bin/manager ships/cmd/manager
    default: Next: Define a resource with:
    default: $ kubebuilder create api
    default: Create Resource under pkg/apis [y/n]?
    default: Create Controller under pkg/controller [y/n]?
    default: Writing scaffold for you to edit...
    default: pkg/apis/ships/v1beta1/sloop_types.go
    default: pkg/apis/ships/v1beta1/sloop_types_test.go
    default: pkg/controller/sloop/sloop_controller.go
    default: pkg/controller/sloop/sloop_controller_test.go
    default: Running make...
    default: go generate ./pkg/... ./cmd/...
    default: go fmt ./pkg/... ./cmd/...
    default: go vet ./pkg/... ./cmd/...
    default: go run vendor/sigs.k8s.io/controller-tools/cmd/controller-gen/main.go all
    default: CRD manifests generated under '/root/go/src/ships/config/crds' 
    default: RBAC manifests generated under '/root/go/src/ships/config/rbac' 
    default: go test ./pkg/... ./cmd/... -coverprofile cover.out
    default: ?   	ships/pkg/apis	[no test files]
    default: ?   	ships/pkg/apis/ships	[no test files]
    default: ok  	ships/pkg/apis/ships/v1beta1	5.351s	coverage: 23.9% of statements
    default: ?   	ships/pkg/controller	[no test files]
    default: ok  	ships/pkg/controller/sloop	7.268s	coverage: 67.6% of statements
    default: ?   	ships/cmd/manager	[no test files]
    default: go build -o bin/manager ships/cmd/manager
    default: go generate ./pkg/... ./cmd/...
    default: go fmt ./pkg/... ./cmd/...
    default: go vet ./pkg/... ./cmd/...
    default: # ships/pkg/apis/ships/pkg/apis/ships/v1beta1
    default: pkg/apis/ships/pkg/apis/ships/v1beta1/zz_generated.deepcopy.go:27:11: undefined: Sloop
    default: Makefile:38: recipe for target 'vet' failed
    default: make: *** [vet] Error 2
The SSH command responded with a non-zero exit status. Vagrant
assumes that this means the command failed. The output for this command
should be in the log above. Please read the output to determine what
went wrong.

@fraenkel
Copy link
Contributor

fraenkel commented Sep 2, 2018

That helped. The issue is I wasn't running make twice to see the actual error. If I run make once, the file is generated in the wrong location.
I haven't been able to detect when the GOPATH isn't set. If I unset my GOPATH and then check the environment variable inside kubebuilder, it is magically set. Either we go with always setting a -o or I need to find a way that will allow me to detect when it is not set.

@fraenkel
Copy link
Contributor

fraenkel commented Sep 3, 2018

Due to a poor choice of dependencies in the controller-tools -> inflect -> gobuffalo, there is an init() method which sets the GOPATH when its missing. The only solution is to compute the relative path to the root of the correct GOPATH entry which would allow movement of the tree. The downside is that this will break if and when modules are supported.

@mheese
Copy link
Author

mheese commented Sep 4, 2018

that's as far as I got with it by now as well :)

hm, it looks to me that we should consider two options:

  1. we are very focused on determining the GOPATH, however, maybe there is another directory path that we could use. And maybe just generating relative paths for -o is already good enough
  2. just go with my original proposed option (1) to document that the GOPATH needs to be set. kubebuilder is a development tool after all, so imho one can be a bit more demanding. However, you are raising a good point about modules.

@fraenkel
Copy link
Contributor

fraenkel commented Sep 5, 2018

Actually using -o is pretty tricky. If you are outside your GOPATH, I am not entirely sure what it will determine the full path from the -o location should look like.
An alternative along a similar line is to modify the Makefile to just fail when its not set because that is the safest option given there is no way to determine the right answer if there is one.

@archisgore
Copy link

I just faced this, and the other issues about paths being all weird (such as NEEDing to be under /usr/local/kubebuilder/bin (I had just copied them under my private ~/bin directory.)

+1 for failing fast in the Makefile. As a pure user, and not contributor, I can and will fix everything you want in my environment, so long as I know what you want. :-) I just always want to know what the tool needs and I'll go make it happen.

@deedubs
Copy link

deedubs commented Oct 17, 2018

I agree with @archisgore, why not just abort early instead of all the mental gymnastics?

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 26, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 26, 2019
@DirectXMan12
Copy link
Contributor

should be fixed a of v2.0.0, closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

No branches or pull requests

8 participants