cleanup hack/verify-govet.sh to throttle process creation #27558
Conversation
|
This patch catches a bunch of govets we are missing currently. Not sure why they are skipped: From 32a216819d4e6356f70fccb1559924bab1c47f6d Mon Sep 17 00:00:00 2001
From: Mike Danese <mikedanese@google.com>
Date: Thu, 16 Jun 2016 11:38:11 -0700
Subject: [PATCH] test more
---
hack/verify-govet.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hack/verify-govet.sh b/hack/verify-govet.sh
index de28e0e..6fce71b 100755
--- a/hack/verify-govet.sh
+++ b/hack/verify-govet.sh
@@ -39,8 +39,8 @@ done
if [[ ${#targets[@]} -eq 0 ]]; then
# Do not run on third_party directories.
- targets=$(go list ./... | egrep -v "/(third_party|vendor)/")
+ targets=$(find . -type f -name '*.go' | egrep -v "/(third_party|vendor)/")
fi
echo "${targets[@]}" \
- | parallel -I {} go vet "${goflags[@]:+${goflags[@]}}" "{}" 2>&1
+ | parallel -I {} go tool vet "${goflags[@]:+${goflags[@]}}" "{}" 2>&1
--
2.8.0.rc3.226.g39d4020Even more if I pass -all to go tool vet |
k8s-github-robot
added
size/M
|
Two questions:
|
|
Ouch on all the 'hidden' results. Is there a place to add a "please |
|
|
||
| exit $failedfiles | ||
| echo "${targets[@]}" \ | ||
| | parallel -I {} go vet "${goflags[@]:+${goflags[@]}}" "{}" 2>&1 |
There was a problem hiding this comment.
it might be simpler just to do | xargs go vet "${goflags[@]:+${goflags[@]}}" here and let go take care of parallizing itself.
on my workstation, the argument list can be very large, so I saw no benefit from using -P. It might be helpful in other environments, possibly.
There was a problem hiding this comment.
also, on my workstation, using xargs vs this shell loop cuts the runtime of this check in half.
|
FYI I filed an issue on |
|
Based on the discussion on that issue, I think Thus we probably want to put a |
mikedanese
force-pushed
the
go-vet-better
branch
from
3aab112 to
1a94cd6
Compare
|
|
||
| exit $failedfiles | ||
| echo "${targets[@]}" \ | ||
| | xargs -P 4 -I {} go vet "${goflags[@]:+${goflags[@]}}" "{}" 2>&1 |
There was a problem hiding this comment.
I think you don't want to use {} here - with {} xargs will spawn one go vet process for each package, rather than one go vet process for all packages.
There was a problem hiding this comment.
woah, how does this possibly work. Is the arg limit a bash thing?
There was a problem hiding this comment.
I think the arg limit is system-dependent, and maybe xargs does the right thing depending on what the limit is?
It's ridiculously large on linux:
$ getconf ARG_MAX
2097152
OSX has a lower limit (I think), so it might be worth seeing if it works correctly there.
mikedanese
force-pushed
the
go-vet-better
branch
from
1a94cd6 to
3144819
Compare
|
@ixdy I could have sworn I was getting an argument list too long but I guess not. Comments addressed. |
|
GCE e2e build/test passed for commit 31448198ceafe61bf0a9b9f698c104d77dbc53f2. |
|
@ixdy can you LGTM this morning? |
mikedanese
added
priority/important-soon
| done | ||
|
|
||
| exit $failedfiles | ||
| go vet "${goflags[@]:+${goflags[@]}}" ${targets[@]} 2>&1 |
There was a problem hiding this comment.
I think the 2>&1 was originally so that grep would work, but it's not that big a deal
ixdy
added
the
lgtm
Running this check as it is on master spikes my load average to 294.19 and looks up my workstation.
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
mikedanese
force-pushed
the
go-vet-better
branch
from
3144819 to
4dd7b7c
Compare
mikedanese
added
lgtm
|
GCE e2e build/test passed for commit 4dd7b7c. |
|
Automatic merge from submit-queue |
Running this check as it is on master spikes my load average to 294.19
and looks up my workstation. Depends on parallel being installed.
cc @thockin @goltermann