Makefile: Provide default hypervisor CMD settings

[bryteise]

As arch detection isn't being done and KNOWN_HYPERVISORS was only
being populated via environment variable settings (and is a runtime
rather than build time dependency anyway) just provide a set of
defaults that can be overridden as needed at build time.

This will also cause any hypervisor configuration to always be
installed.

[grahamwhaley]

quick feedback - you'll need to modify the PR to match the project format/requirements to pass the initial CI - see link in the travis logs as per https://github.com/kata-containers/community/blob/master/CONTRIBUTING.md#patch-format below:

Found 1 commit between commit 9dc3e5e56c18d8cb483924ed98815445ae949fbc and branch master
ERROR: No "Fixes" found
ERROR: checkcommits failed. See the document below for help on formatting
commits for the project.
https://github.com/kata-containers/community/blob/master/CONTRIBUTING.md#patch-format
The command ".ci/static-checks.sh" failed and exited with 1 during .

[bryteise]

@grahamwhaley Thanks for the pointer, I had just tossed it up and was looking to see what stuck. I'll open an issue as that seems like the only thing I'm failing on with the patch-format now after fixing the signoff.

[amshinde]

/test

[amshinde]

lgtm

[jodh-intel]

Thanks for raising but either I'm misunderstanding what your PR does, or you are misunderstanding what our build system does :)

The way it currently works is to load arch-specific settings from arch/*.mk. That sets the correct QEMUCMD (which is architecture dependent btw) and also sets FCCMD for those architectures that FC supports.

Providing a default qemu binary name is meaningless if you look at how they vary by platform.

[jodh-intel]

What's this? We don't use a kata prefix as this is not a Kata-developed component.

[jodh-intel]

Ah - I think I might see what you were trying to do now; the build currently requires GOPATH. To resolve that, something like the following would work:

diff --git a/Makefile b/Makefile
index 1c11f74..86ce694 100644
--- a/Makefile
+++ b/Makefile
@@ -23,22 +23,20 @@ ifeq ($(SKIP_GO_VERSION_CHECK),)
     include golang.mk
 endif
 
-ifneq ($(GOPATH),)
-    GOARCH=$(shell go env GOARCH)
-    ifeq ($(ARCH),)
-        ARCH = $(GOARCH)
-    endif
-
-    ARCH_DIR = arch
-    ARCH_FILE_SUFFIX = -options.mk
-    ARCH_FILE = $(ARCH_DIR)/$(ARCH)$(ARCH_FILE_SUFFIX)
-    ARCH_FILES = $(wildcard arch/*$(ARCH_FILE_SUFFIX))
-    ALL_ARCHES = $(patsubst $(ARCH_DIR)/%$(ARCH_FILE_SUFFIX),%,$(ARCH_FILES))
-
-    # Load architecture-dependent settings
-    include $(ARCH_FILE)
+GOARCH=$(shell go env GOARCH)
+ifeq ($(ARCH),)
+    ARCH = $(GOARCH)
 endif
 
+ARCH_DIR = arch
+ARCH_FILE_SUFFIX = -options.mk
+ARCH_FILE = $(ARCH_DIR)/$(ARCH)$(ARCH_FILE_SUFFIX)
+ARCH_FILES = $(wildcard arch/*$(ARCH_FILE_SUFFIX))
+ALL_ARCHES = $(patsubst $(ARCH_DIR)/%$(ARCH_FILE_SUFFIX),%,$(ARCH_FILES))
+
+# Load architecture-dependent settings
+include $(ARCH_FILE)
+
 PROJECT_TYPE = kata
 PROJECT_NAME = Kata Containers
 PROJECT_TAG = kata-containers

[bryteise]

@jodh-intel So the issue raised is when the GOPATH is set, the autodetection using the arch scripts isn't run is that something that could be modified then?

[bryteise]

@jodh-intel Okay, I'll give that a try.

[jcvenegas]

@jodh-intel I tested your patch, it as I was expecting, what I dont really get is that the GOPATH is actually set en my environment

[bryteise]

@jodh-intel Okay, I just applied your suggestion directly and am happy with the result, thanks!

[jodh-intel]

Thanks @bryteise.

lgtm

[jodh-intel]

It might be worth adding a comment here explaining this is to allow the user to set ARCH before we overwrite it.

More info

A quick look makes it appear as though this is wrong since ARCH isn't set yet. However, since ARCH is added to the USER_VARS variable, that check is to allow the user to define the architecture they want to build with (cross build). If not specified, we default to the host architecture (using go's naming rather than that of arch(1)):

$ make ARCH=foo ...

[devimc]

lgtm

[devimc]

/test

[jodh-intel]

/retest

[jodh-intel]

fedora vsocks CI failed due to network timeout. But since all the other CI's are passing (ignoring the known ARM issue), I'm going to merge anyway since this code is not distro specific...

[teawater]

Ah - I think I might see what you were trying to do now; the build currently requires GOPATH. To resolve that, something like the following would work:

diff --git a/Makefile b/Makefile
index 1c11f74..86ce694 100644
--- a/Makefile
+++ b/Makefile
@@ -23,22 +23,20 @@ ifeq ($(SKIP_GO_VERSION_CHECK),)
     include golang.mk
 endif
 
-ifneq ($(GOPATH),)
-    GOARCH=$(shell go env GOARCH)
-    ifeq ($(ARCH),)
-        ARCH = $(GOARCH)
-    endif
-
-    ARCH_DIR = arch
-    ARCH_FILE_SUFFIX = -options.mk
-    ARCH_FILE = $(ARCH_DIR)/$(ARCH)$(ARCH_FILE_SUFFIX)
-    ARCH_FILES = $(wildcard arch/*$(ARCH_FILE_SUFFIX))
-    ALL_ARCHES = $(patsubst $(ARCH_DIR)/%$(ARCH_FILE_SUFFIX),%,$(ARCH_FILES))
-
-    # Load architecture-dependent settings
-    include $(ARCH_FILE)
+GOARCH=$(shell go env GOARCH)
+ifeq ($(ARCH),)
+    ARCH = $(GOARCH)
 endif
 
+ARCH_DIR = arch
+ARCH_FILE_SUFFIX = -options.mk
+ARCH_FILE = $(ARCH_DIR)/$(ARCH)$(ARCH_FILE_SUFFIX)
+ARCH_FILES = $(wildcard arch/*$(ARCH_FILE_SUFFIX))
+ALL_ARCHES = $(patsubst $(ARCH_DIR)/%$(ARCH_FILE_SUFFIX),%,$(ARCH_FILES))
+
+# Load architecture-dependent settings
+include $(ARCH_FILE)
+
 PROJECT_TYPE = kata
 PROJECT_NAME = Kata Containers
 PROJECT_TAG = kata-containers

@jodh-intel does makefile call "go" without GOPATH? If so, maybe I can update "GOPATH not set mode" to non-go mode because:

$ sudo go
sudo: go: command not found

[jodh-intel]

I think you probably want sudo -E go - that's what we do in our CI. I personally set GOPATH to make my life easier ;)