dockerfile: arg for controlling go build flags #3994
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This introduces a new `make test-race` target, which will compile buildkitd using the `go build -race` flag, which is useful for detecting data races. Signed-off-by: Alex Couture-Beil <alex@earthly.dev>
|
here's a sample run against a specific test: which replicates a data race: |
alexcb
commented
Jul 6, 2023
| @@ -34,6 +34,10 @@ clean: | |||
| test: | |||
| ./hack/test integration gateway dockerfile | |||
|
|
|||
| .PHONY: test-race | |||
| test-race: | |||
There was a problem hiding this comment.
Maybe this new target is overkill, and it's sufficient to just indicate the new CGO_ENABLED=1 BUILDFLAGS="-race" envs in the CONTRIBUTING.md?
|
Adding this to all PRs might be overkill, but perhaps there's a limited number of integration tests that could be called via github actions (once the existing data races are fixed, see #3862 for instance)? |
crazy-max
reviewed
Jul 6, 2023
Dockerfile
Outdated
Comment on lines
98
to
103
| ARG GO_RACE_ENABLED | ||
| RUN --mount=target=. --mount=target=/root/.cache,type=cache \ | ||
| --mount=target=/go/pkg/mod,type=cache \ | ||
| --mount=source=/tmp/.ldflags,target=/tmp/.ldflags,from=buildkit-version \ | ||
| CGO_ENABLED=0 xx-go build -ldflags "$(cat /tmp/.ldflags) -extldflags '-static'" -tags "osusergo netgo static_build seccomp ${BUILDKITD_TAGS}" -o /usr/bin/buildkitd ./cmd/buildkitd && \ | ||
| xx-verify --static /usr/bin/buildkitd | ||
| CGO_ENABLED="$GO_RACE_ENABLED" xx-go build $(if [ "$GO_RACE_ENABLED" != "0" ]; then echo -race; fi) -ldflags "$(cat /tmp/.ldflags) -extldflags '-static'" -tags "osusergo netgo static_build seccomp ${BUILDKITD_TAGS}" -o /usr/bin/buildkitd ./cmd/buildkitd && \ | ||
| if [ "$GO_RACE_ENABLED" = "0" ]; then xx-verify --static /usr/bin/buildkitd; fi |
There was a problem hiding this comment.
Could we have a generic name for this, maybe BUILDFLAGS empty by default? Might be useful in the future. Same for CGO_ENABLED defaulting to 0 or we could detect if -race is being passed and set the right value?
ARG BUILDFLAGS
ARG CGO_ENABLED=0
RUN --mount=target=. --mount=target=/root/.cache,type=cache \
--mount=target=/go/pkg/mod,type=cache \
--mount=source=/tmp/.ldflags,target=/tmp/.ldflags,from=buildkit-version \
xx-go build ${BUILDFLAGS} -ldflags "$(cat /tmp/.ldflags) -extldflags '-static'" -tags "osusergo netgo static_build seccomp ${BUILDKITD_TAGS}" -o /usr/bin/buildkitd ./cmd/buildkitd && \
xx-verify --static /usr/bin/buildkitdThere was a problem hiding this comment.
Good idea; I also introduced a VERIFYFLAGS which is used to prevent the xx-verify --static ... call, by explicitly doing export VERIFYFLAGS="" (since --race requires cgo which is not static).
If there's a use-case for allowing users to change the VERIFYFLAGS, we could make the bash code allow users to set it (and maybe error out if it contains -static).
alexcb
changed the title
alexcb
force-pushed
the
acb/race
branch
2 times, most recently
from
c63fb17
to
a92c55f
Compare
This introduces new `BUILDFLAGS`, `VERIFYFLAGS`, and `CGO_ENABLED` build-args,
which can be used to change how buildkitd is compiled.
This, for example, can be used to enable the go data race detector during
integration testing:
CGO_ENABLED=1 BUILDFLAGS="-race" TESTFLAGS="-v -test.run=TestClientGatewayIntegration/TestClientGatewaySolve" TESTPKGS=./client ./hack/test integration
This additionally introduces a new `make test-race` target, which
simplifies how to run all tests with the race detector.
Signed-off-by: Alex Couture-Beil <alex@earthly.dev>
tonistiigi
approved these changes
Jul 7, 2023
Signed-off-by: Alex Couture-Beil <alex@earthly.dev>
tonistiigi
approved these changes
Jul 7, 2023
Signed-off-by: Alex Couture-Beil <alex@earthly.dev>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This introduces new
BUILDFLAGS,VERIFYFLAGS, andCGO_ENABLEDbuild-args,which can be used to change how buildkitd is compiled.
This, for example, can be used to enable the go data race detector during
integration testing:
This additionally introduces a new
make test-racetarget, whichsimplifies how to run all tests with the race detector.
Signed-off-by: Alex Couture-Beil alex@earthly.dev