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

Makefile: Fix errors when specifying RACE #11631

Conversation

christarazi
Copy link
Member

See commit msgs.

Fixes:

```
$ sudo -E make SKIP_VET=true SKIP_KVSTORES=true TESTPKGS=pkg/aws/eni RACE=1 unit-tests
dirname: missing operand
Try 'dirname --help' for more information.
make  -C tools/maptool/
make[1]: Entering directory
'/home/chris/code/cilium/cilium/tools/maptool'
CGO_ENABLED=0 go build -mod=vendor -race -ldflags '-X
"github.com/cilium/cilium/pkg/version.Version=1.7.90 2d4cc76
2020-05-20T12:25:41-07:00 go version go1.14.2 linux/amd64" -s -w -X
"github.com/cilium/cilium/pkg/envoy.RequiredEnvoyVersionSHA=a3385205ad620550b35d3b0b651e40898386e6e3"
-X
"github.com/cilium/cilium/pkg/datapath/loader.DatapathSHA=7359bebb8825a116e3b022e9a3ff35045d0709ce"
'  -o maptool
go build: -race requires cgo; enable cgo by setting CGO_ENABLED=1
make[1]: *** [Makefile:14: maptool] Error 2
make[1]: Leaving directory
'/home/chris/code/cilium/cilium/tools/maptool'
make: *** [Makefile:216: unit-tests] Error 2
```

Signed-off-by: Chris Tarazi <chris@isovalent.com>
Fixes:

```
$ sudo -E make SKIP_VET=true SKIP_KVSTORES=true TESTPKGS=pkg/aws/eni RACE=1 unit-tests
make  -C tools/maptool/
..
echo "mode: count" > coverage-all-tmp.out
echo "mode: count" > coverage.out
...
for pkg in github.com/cilium/cilium/pkg/aws/eni; do \
        go test -mod=vendor -race -ldflags "-X github.com/cilium/cilium/pkg/kvstore.consulDummyConfigFile=/tmp/cilium-consul-certs/cilium-consul.yaml -X github.com/cilium/cilium/pkg/testutils.CiliumRootDir=/home/chris/code/cilium/cilium -X github.com/cilium/cilium/pkg/datapath.DatapathSHA=1234567890abcdef7890 -X github.com/cilium/cilium/pkg/logging.DefaultLogLevelStr="error"" $pkg -test.v -timeout 360s -coverprofile=coverage.out -covermode=count -coverpkg github.com/cilium/cilium/pkg/aws/eni \
        || exit 1; \
        tail -n +2 coverage.out >> coverage-all-tmp.out; \
done
-covermode must be "atomic", not "count", when -race is enabled
make: *** [Makefile:224: unit-tests] Error 1

```

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi requested a review from a team as a code owner May 21, 2020 00:35
@christarazi christarazi added area/build Anything to do with the build, more general then area/CI area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. release-note/misc This PR makes changes that have no direct user impact. labels May 21, 2020
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@christarazi
Copy link
Member Author

christarazi commented May 21, 2020

test-me-please

Edit: runtime hit #11512

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 37.088% when pulling 8c2725d on christarazi:pr/christarazi/fix-makefile-with-race-enabled into 2d4cc76 on cilium:master.

@christarazi
Copy link
Member Author

retest-runtime

@christarazi christarazi added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 21, 2020
@joestringer joestringer merged commit 59d1412 into cilium:master May 21, 2020
1.8.0 automation moved this from In progress to Merged May 21, 2020
@christarazi christarazi deleted the pr/christarazi/fix-makefile-with-race-enabled branch May 21, 2020 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Anything to do with the build, more general then area/CI area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

4 participants