PSS Measurement:
Qemu: 165581 KB
Proxy: 4161 KB
Shim: 8903 KB

Memory inside container:
Total Memory: 2043460 KB
Free Memory: 2006216 KB

Codecov Report

Merging #762 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #762   +/-   ##
=======================================
  Coverage   65.27%   65.27%           
=======================================
  Files          87       87           
  Lines       10554    10554           
=======================================
  Hits         6889     6889           
  Misses       2973     2973           
  Partials      692      692

I agree that we should use newer golang releases. There are several lock os thread related fixes in the 1.10 release.

But I'm not sure if the source code is the right way to add the restriction. How about adding it to the Makefile instead?

But I'm not sure if the source code is the right way to add the restriction. How about adding it to the Makefile instead?

This is the easier way to block user from compling it with an old Golang version.
Any good idea relating to your Makefile? Some golang version judgement?

@WeiZhang555 Yes, I was thinking about adding go version check in the Makefile. It will be easier to find in future, assuming we'll change it again some time later. And it doesn't need to compile a few files and then fail when reaching cli/oci.go. wdyt?

I quite like the code addition - but, agree that it will be easier to find and trigger earlier if in the Makefile.

@WeiZhang555 I think what @bergwolf concerns is, next time, if we want to add +build go1.11, how could we find this tag and update it rather than add it in another source file.

Yes, I was thinking about adding go version check in the Makefile

Both OK for me. Let's have a try...

Travis is having a big snooze - I don't see a build running at https://travis-ci.org/kata-containers/runtime/builds

oops, wrong PR - that travis comment was aimed at #764...

I'm a terrible Makefile writer, just tried and didn't get a nice result, I think I need to ask for some help here, does anyone want to pick up the Makefile part? @bergwolf Any interest? 😂

@WeiZhang555 Here is an example of how it's done in other projects. It's a bit complicated but feel free to borrow the script from there since ipfs is MIT license (which allows redistribution under a different license).

/me looks to @jodh-intel as a Makefile god to do review and input ;-)

How about something like this (but note the FIXME in there :)... ?

diff --git a/Makefile b/Makefile
index 96b26dc..98591e0 100644
--- a/Makefile
+++ b/Makefile
@@ -15,6 +15,8 @@ done)
 GOARCH=$(shell go env GOARCH)
 HOST_ARCH=$(shell arch)
 
+include golang.mk
+
 ifeq ($(ARCH),)
 	ARCH = $(GOARCH)
 endif
diff --git a/golang.mk b/golang.mk
new file mode 100644
index 0000000..6cadfdd
--- /dev/null
+++ b/golang.mk
@@ -0,0 +1,37 @@
+#
+# Copyright (c) 2018 Intel Corporation
+#
+# SPDX-License-Identifier: Apache-2.0
+#
+
+# FIXME: this should be a query of versions.yaml (languages.golang.version)
+golang_version_min_major = 1
+golang_version_min_minor = 10
+
+# for error messages
+golang_version_needed=$(golang_version_min_major).$(golang_version_min_minor)
+
+golang_version_raw=$(shell go version 2>/dev/null)
+
+ifeq (,$(golang_version_raw))
+    $(error "ERROR: cannot determine golang version")
+endif
+
+# determine actual version of golang
+golang_version=$(subst go,,$(word 3,$(golang_version_raw)))
+
+golang_version_fields=$(subst ., ,$(golang_version))
+
+golang_version_major=$(word 1,$(golang_version_fields))
+golang_version_minor=$(word 2,$(golang_version_fields))
+
+golang_major_ok=$(shell test $(golang_version_major) -ge $(golang_version_min_major) && echo ok)
+golang_minor_ok=$(shell test $(golang_version_major) -eq $(golang_version_min_major) -a $(golang_version_minor) -ge $(golang_version_min_minor) && echo ok)
+
+ifeq (,$(golang_major_ok))
+    $(error "ERROR: golang major version too old: got $(golang_version), need atleast $(golang_version_needed)")
+endif
+
+ifeq (,$(golang_minor_ok))
+    $(error "ERROR: golang minor version too old: got $(golang_version), need atleast $(golang_version_needed)")
+endif

@WeiZhang555 - just a reminder - are you OK with the Makefile fragment from @jodh-intel If you are not able to apply, maybe we can pick this up for you?

@grahamwhaley The Makefile looks quite good. Yes, you can pick this up for me, since I am in vacation in next 3 weeks. Thank you!

@jodh-intel - for the Makefile FIXME, I've added:

golang_version_min=$(shell yq r versions.yaml languages.golang.version)

ifeq (,$(golang_version_min))
    $(error "ERROR: cannot determine minimum golang version")
endif

golang_version_min_fields=$(subst ., ,$(golang_version_min))

golang_version_min_major=$(word 1,$(golang_version_min_fields))
golang_version_min_minor=$(word 2,$(golang_version_min_fields))

and that seems to work - see any issues with that? (apart from we then require yq, but hey ho, we should fail the build without it cleanly).

I'll put together a new PR

actually, whilst there @jodh-intel , I was a little thrown by the intention of the golang meta data in the yaml - any insights welcome :-) I was going to bump the version to 1.10.4, as listed on golang.org at present. What do I do with the meta.newest-version ?? is that a restriction/cap on the max version we should use - in which case we (I) can encode that lookup into the Makefile as well whilst we are there.

  golang:
    description: "Google's 'go' language"
    notes: "'version' is the default minimum version used by this project."
    version: "1.10.4"
    meta:
      newest-version: "1.10"

@grahamwhaley - the original intention was:

• languages.golang.version: minimum required version
• languages.golang.meta.newest-version: newest version that we know to work (not necessarily the newest available version).

The ideal would be that we'd test both. However, the tests and packaging are currently using languages.golang.meta.newest-version (only, afaik) [1].

Feel free to add a comment in the YAML (you could even add languages.golang.meta.newest-version-description or something [2].

[1] - Note that we can easily update the the newest-version simply by changing the value in versions.yaml in a PR since the CI will read it and install that version of golang.

[2] - That's a bit hacky as ideally we'd make it languages.golang.meta.newest. and add version and description below it with an anchor to keep compat with the current languages.golang.meta.newest-version.

@grahamwhaley - Your yq usage seems fine to me, although strictly yq should then be added to:

• https://github.com/kata-containers/documentation/blob/master/Developer-Guide.md#requirements-to-build-individual-components

The only question is which version yq should be reading from versions.yaml :)

@WeiZhang555 ping.

Close this as we have a better pr #806 :-)

Just came across this and noticed that the issue number is wrong. No idea what it should be, but it's not #148 ;)

Actually, it might be #148 - see #806 and
#148 (comment)

Unless we find something got closed that should not - it all feels benign now anyway.

Hmm. Too late to change it now, but I think a separate issue would have been best, linked to from the PR and #148.