-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Standardize hubble and cilium CLIs makefile #37716
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
Conversation
37a7e8c
to
b250004
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @HadrienPatte 👍 couple of comments, nothing blocking so overall LGTM.
b250004
to
646812b
Compare
3596eb6
to
4adde52
Compare
cilium-cli change: non stripped binaries that were previously built with DEBUG=1 make -C cilium-cli are now built with NOSTRIP=1 make -C cilium-cli Signed-off-by: Hadrien Patte <hadrien.patte@datadoghq.com>
/test |
I think we pulled in the relevant reviewers here. Looking back over maybe we could change the ownership of all Makefiles over to @cilium/build to ease the review for these changes in future. I don't think we'll need to get Andrew's review. |
BINDIR = /usr/local/bin | ||
CLI_VERSION=$(shell git describe --tags --always) | ||
CLI_MAIN_DIR?=./cmd/cilium | ||
STRIP_DEBUG=-w -s | ||
ifdef DEBUG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noted that this changes the build infra for how we handle this env flag DEBUG
, but I couldn't find any traces where this flag is used. Just leaving a note here that the equivalent will now be to set NOSTRIP
. Seems like this was inherited from https://github.com/cilium/cilium-cli anyway.
Thanks for the cleanup @HadrienPatte ! |
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXX
line if the commit addresses a particularGitHub issue.
Fixes: <commit-id>
tag, thenplease add the commit author[s] as reviewer[s] to this issue.
Most
Makefile
s in this repository follow a standard pattern, inheriting a lot of their config from the root'sMakefile.defs
. Thecilium-cli
andhubble
directories Makefiles are notable exceptions. This PR standardizes them to remove duplicated code and logic and to make them behave more like other Makefiles.Note: this PR is similar with/related to/a followup of #37911
Summary of changes:
cilium-cli/Makefile
:Makefile.defs
:GO
,GO_BUILD
,GO_TAGS
STRIP_DEBUG
andDEBUG
, the behavior they controlled is now controlled viaNOSTRIP
$(GO_TEST)
instead of$(GO) test
.DEFAULT_GOAL
and.PHONY
targetsImpact on
make -C cilium-cli
:Before:
After:
Difference are the addition of
-mod=vendor
and-tags=osusergo
as well as cilium version variables (currently unused by cilium-cli)Note: to build the cilium-cli binary with debug symbols unstripped, we now should use
NOSTRIP=1 make -C cilium-cli
instead of the previousDEBUG=1 make -C cilium-cli
hubble/Makefile
:Makefile.defs
:GO
,GO_BUILD_FLAGS
,GO_TEST_FLAGS
,GO_BUILD
,VERSION
,GO_TAGS
hubble
target with$(TARGET)
make
with$(MAKE)
.DEFAULT_GOAL
and.PHONY
targetsImpact on
make -C hubble
:Before:
After:
Difference is that the two separate
-ldflags
args are now combined as a single one.operator/Makefile
:GOCLEAN
variable and replace$(GO) clean
with$(GO_CLEAN)
.DEFAULT_GOAL
and.PHONY
targetsNote: the
GO_BUILD
variable fromMakefile.defs
already automatically wires up the go build flags and ldflags, simplifying usage.