Gometalinter fixups for non-x86#34759
Conversation
|
I have just added one more commit. Quoting the changelog:
|
|
The only remaining issue I see is UPDATE this is a known issue fixed in go1.9 |
hack/validate/gometalinter.json
Outdated
There was a problem hiding this comment.
This can be overridden on the command line. Maybe we can set an environment variable for arm and keep the lower deadline? The environment variable can be used to set the command line flag in hack/validate/gometalinter
Really this should be able to run in a few seconds, so if it's this slow there is a problem somewhere.
There was a problem hiding this comment.
Indeed it takes under a minute on my (quite powerful) laptop, but about 6 minutes on a Scaleway C1 machine (which is in general pretty slow, as it's single 32-bit ARM CPU).
What about something like this:
diff --git a/hack/validate/gometalinter b/hack/validate/gometalinter
index 9830659d6..596370826 100755
--- a/hack/validate/gometalinter
+++ b/hack/validate/gometalinter
@@ -1,6 +1,18 @@
#!/usr/bin/env bash
set -e -o pipefail
+# Timeout is different depending on the arch. Values below are empirical.
+case $(uname -m) in
+ armv7l)
+ TIMEOUT=10m
+ ;;
+ *)
+ TIMEOUT=2m
+ ;;
+esac
+
SCRIPTDIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
-gometalinter --config $SCRIPTDIR/gometalinter.json ./...
+gometalinter --deadline "$TIMEOUT" --config $SCRIPTDIR/gometalinter.json ./...There was a problem hiding this comment.
That could work, but is the timeout really dependent on the arch, or is it more about the particular host?
Maybe we can just do --deadline "${GOMETALINTER_TIMEOUT-2m}" and set that env value in Dockerfile.armhf ?
There was a problem hiding this comment.
OK, will redo. I also discovered that a run on AARCH64 does not fit into 2m either.
0f27d96 to
59d4f8a
Compare
|
The |
ec13110 to
42b1369
Compare
Since commit d7e2c4c ("Use gometalinter for linting") command "make all" fails on all the non-default platforms (i.e. ARMs, PPC, and s390) in this way: # make all ... Congratulations! All commits are properly signed with the DCO! /go/src/github.com/docker/docker/hack/validate/gometalinter: line 6: gometalinter: command not found Makefile:105: recipe for target 'all' failed make: *** [all] Error 127 Make sure gometalinter is installed for those platforms Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This commit reverts a hunk of commit 2f5f0af ("Add unconvert linter") and adds a hint for unconvert linter to ignore excessive conversion as it is required on 32-bit platforms (e.g. armhf). The exact error on armhf is this: 19:06:45 ---> Making bundle: dynbinary (in bundles/17.06.0-dev/dynbinary) 19:06:48 Building: bundles/17.06.0-dev/dynbinary-daemon/dockerd-17.06.0-dev 19:10:58 # github.com/docker/docker/daemon/graphdriver/overlay 19:10:58 daemon/graphdriver/overlay/copy.go:161: cannot use stat.Atim.Sec (type int32) as type int64 in argument to time.Unix 19:10:58 daemon/graphdriver/overlay/copy.go:161: cannot use stat.Atim.Nsec (type int32) as type int64 in argument to time.Unix 19:10:58 daemon/graphdriver/overlay/copy.go:162: cannot use stat.Mtim.Sec (type int32) as type int64 in argument to time.Unix 19:10:58 daemon/graphdriver/overlay/copy.go:162: cannot use stat.Mtim.Nsec (type int32) as type int64 in argument to time.Unix Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
I have run into two separate issues while doing 'make all' on armhf (a Scaleway C1 machine, same as used in CI). This commit fixes both. 1. There were a lot of "not enough memory" errors, and after that in a few runs gometalinter just stuck forever on FUTEX_WAIT with no children left. Looking into docs, I found the --enable-gc option which solved the issue. [Update: this has already been added] 2. Timeout of 2 minutes is not enough for the abovementioned platform. The longest running linter is goimports which takes almost 6 minutes to run. Set the timeout to the observable run time roughly doubled. In addition, ARM platforms does not have too much RAM (2GB), so running too many processes in parallel might be problematic. Limit it by using -j2 [v2: make the timeout arch-dependent, also tested on aarch64 (2m15s)] [v3: moved timeout setting to Dockerfiles] [v4: generalized to GOMETALINTER_OPTS, added -j2 for ARM platforms] [v5: rebase to master] Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is mostly to include the following fix: alecthomas/gometalinter@78e3fbd90a20b03a Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
42b1369 to
6be4b37
Compare
|
Does this depend on upgrading to Go 1.9 first? |
|
I don't think so, why would it? |
| gometalinter --config $SCRIPTDIR/gometalinter.json ./... | ||
| # CI platforms differ, so per-platform GOMETALINTER_OPTS can be set | ||
| # from a platform-specific Dockerfile, otherwise let's just set | ||
| # (somewhat pessimistic) default of 10 minutes. |
|
I saw this comment; that's why I wondered:
|
|
@thaJeztah it's just I found a (not quite related) issue with a linter which is fixed in Go 1.9. This is not a regression caused by these commits. The overall reason for this series is more chances for |
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM (sorry already thought I did, 😊)
|
All tests passed 😅 |
TwoThree patches fixing things broken for non-x86 platforms after introducing gometalinter and additional linters. Please see commit messages.