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

Building drivers crashes on missing translations #4909

Closed
afbjorklund opened this issue Jul 29, 2019 · 10 comments
Closed

Building drivers crashes on missing translations #4909

afbjorklund opened this issue Jul 29, 2019 · 10 comments
Labels
area/build-release co/hyperkit Hyperkit related issues co/kvm2-driver KVM2 driver related issues

Comments

@afbjorklund
Copy link
Collaborator

afbjorklund commented Jul 29, 2019

When trying to build either of the machine drivers, the build fails due to missing translations:

.PHONY: drivers
drivers: out/docker-machine-driver-hyperkit out/docker-machine-driver-kvm2
# k8s.io/minikube/pkg/minikube/translate
pkg/minikube/translate/translate.go:77:12: undefined: Asset

It requires this file to be generated (from JSON):
pkg/minikube/translate/translations.go

These files should not be needed nor used in the drivers, that are supposed to be lowlevel...

i.e. I don't think that we should have translation support, in the docker machine drivers code

@afbjorklund
Copy link
Collaborator Author

@medyagh @sharifelgamal anyone knows what is going here ?
Is go build flipping out because of using modules, or what ?

$ go build -v k8s.io/minikube/cmd/drivers/kvm
k8s.io/minikube/pkg/minikube/translate
# k8s.io/minikube/pkg/minikube/translate
pkg/minikube/translate/translate.go:77:12: undefined: Asset

is there a good tool to show dependencies ?

@afbjorklund afbjorklund added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Jul 29, 2019
@afbjorklund
Copy link
Collaborator Author

Using go list -json is not so bad, it shows all the dependencies:

GOOS=darwin go list -json cmd/drivers/hyperkit/main.go

GOOS=linux go list -json cmd/drivers/kvm/main.go

It is the usage of either of these two imports:

  • k8s.io/minikube/pkg/minikube/config

  • k8s.io/minikube/pkg/util

@afbjorklund
Copy link
Collaborator Author

I added the translations as a dependency, but don't really like it.

It is definitely going to block any upstreaming... 😢 #3939 #3169

@josedonizetti
Copy link
Member

@afbjorklund I've just tried to build the kvm driver on master and it worked without problem with both make out/docker-machine-driver-kvm2 and go build -v k8s.io/minikube/cmd/drivers/kvm.
Is this error related to the link PR?

@afbjorklund
Copy link
Collaborator Author

afbjorklund commented Jul 29, 2019

It is related so far as I started building the drivers clean, and noticed the issue when the files are gone.

Try removing the generated pkg/minikube/translate/translations.go file before you build the driver

@afbjorklund afbjorklund removed the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Jul 29, 2019
@josedonizetti
Copy link
Member

@afbjorklund
Copy link
Collaborator Author

afbjorklund commented Jul 29, 2019

The "constants" are OK, they're just hiding some strings. Will be a problem when upstreaming, though.

constants.DriverHyperkit ("hyperkit")
constants.DriverKvm2 ("kvm2")

And some extra defaults:
constants.DefaultDiskSize
constants.DefaultMemorySize
constants.DefaultCPUS
constants.DefaultISOURL
constants.GetMinipath

The problem here is using minikube constants, in what is supposed to be a stand-alone machine driver.

@josedonizetti
Copy link
Member

josedonizetti commented Jul 29, 2019

@afbjorklund Agree. But constants are really simple to extract. Currently, the driver is using a function that depends on the translations directly util.CalculateSizeInMB(...):
https://github.com/kubernetes/minikube/blob/master/pkg/drivers/kvm/kvm.go#L107-L108
https://github.com/kubernetes/minikube/blob/master/pkg/util/utils.go#L63

@afbjorklund
Copy link
Collaborator Author

Currently, the driver is using a function that depends on the translations directly util.CalculateSizeInMB(...):

Yeah, someone desperately wanted to tag a "mb" onto the default value...

The original driver only had these default as CLI flags, not in the driver code itself.
https://github.com/dhiltgen/docker-machine-kvm/blob/master/kvm.go#L102_L116

I can't really see why parsing the "millibytes" should be locale-specific, though.
(or why we can't spell MB, but maybe that is for our Windows friends or something)

But I think I will just leave it (for now), otherwise someone will want "mebibytes".

@afbjorklund
Copy link
Collaborator Author

Fixed in ea4aeaf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build-release co/hyperkit Hyperkit related issues co/kvm2-driver KVM2 driver related issues
Projects
None yet
Development

No branches or pull requests

2 participants