Skip to content

Conversation

HadrienPatte
Copy link
Member

@HadrienPatte HadrienPatte commented Feb 18, 2025

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!
Standardize hubble and cilium CLIs makefile

Most Makefiles in this repository follow a standard pattern, inheriting a lot of their config from the root's Makefile.defs. The cilium-cli and hubble 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:

  • Remove duplicated variables already sourced from Makefile.defs: GO, GO_BUILD, GO_TAGS
  • Remove variable STRIP_DEBUG and DEBUG, the behavior they controlled is now controlled via NOSTRIP
  • Use $(GO_TEST) instead of $(GO) test
  • Update/Add .DEFAULT_GOAL and .PHONY targets

Impact on make -C cilium-cli:
Before:

make: Entering directory '/home/hadrien/go/src/github.com/DataDog/cilium/cilium-cli'
CGO_ENABLED=0 go build  \
        -ldflags "-w -s -X 'github.com/cilium/cilium/cilium-cli/defaults.CLIVersion=v1.17.0-pre.3-1281-ge8ee0f0770'" \
        -o cilium \
        ./cmd/cilium
make: Leaving directory '/home/hadrien/go/src/github.com/DataDog/cilium/cilium-cli'

After:

make: Entering directory '/home/hadrien/go/src/github.com/DataDog/cilium/cilium-cli'
CGO_ENABLED=0 go build  -mod=vendor -ldflags ' -X "github.com/cilium/cilium/pkg/version.ciliumVersion=1.18.0-dev bbd15b1532 2025-02-18T15:24:14+01:00" -s -w -X "github.com/cilium/cilium/pkg/envoy.requiredEnvoyVersionSHA=c3c35d52ca3b699de1f9448ab7174a9bdcb13f69" -X "github.com/cilium/cilium/cilium-cli/defaults.CLIVersion=v1.17.0-pre.3-1282-gbbd15b1532" ' -tags=osusergo  \
        -o cilium \
        ./cmd/cilium
make: Leaving directory '/home/hadrien/go/src/github.com/DataDog/cilium/cilium-cli'

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 previous DEBUG=1 make -C cilium-cli

hubble/Makefile:

  • Add missing copyright header
  • Remove duplicated variables already sourced from Makefile.defs: GO, GO_BUILD_FLAGS, GO_TEST_FLAGS, GO_BUILD, VERSION, GO_TAGS
  • Replace hardcoded references to the hubble target with $(TARGET)
  • Replace make with $(MAKE)
  • Update/Add .DEFAULT_GOAL and .PHONY targets

Impact on make -C hubble:
Before:

make: Entering directory '/home/hadrien/go/src/github.com/DataDog/cilium/hubble'
GOOS= GOARCH= CGO_ENABLED=0 go build  -mod=vendor -ldflags ' -X "github.com/cilium/cilium/pkg/version.ciliumVersion=1.18.0-dev e8ee0f0770 2025-02-28T14:15:28+01:00" -s -w -X "github.com/cilium/cilium/pkg/envoy.requiredEnvoyVersionSHA=c3c35d52ca3b699de1f9448ab7174a9bdcb13f69" ' -tags=osusergo   -ldflags "-w -s -X 'github.com/cilium/cilium/hubble/pkg.GitBranch=main' -X 'github.com/cilium/cilium/hubble/pkg.GitHash=e8ee0f0770' -X 'github.com/cilium/cilium/hubble/pkg.Version=v1.18.0-dev'" -o ./hubble .
make: Leaving directory '/home/hadrien/go/src/github.com/DataDog/cilium/hubble'

After:

make: Entering directory '/home/hadrien/go/src/github.com/DataDog/cilium/hubble'
GOOS= GOARCH=amd64 CGO_ENABLED=0 go build  -mod=vendor -ldflags ' -X "github.com/cilium/cilium/pkg/version.ciliumVersion=1.18.0-dev bbd15b1532 2025-02-18T15:24:14+01:00" -s -w -X "github.com/cilium/cilium/pkg/envoy.requiredEnvoyVersionSHA=c3c35d52ca3b699de1f9448ab7174a9bdcb13f69" -X "github.com/cilium/cilium/hubble/pkg.GitBranch=makefile" -X "github.com/cilium/cilium/hubble/pkg.GitHash=bbd15b1532" -X "github.com/cilium/cilium/hubble/pkg.Version=v1.18.0-dev" ' -tags=osusergo  -o ./hubble .
make: Leaving directory '/home/hadrien/go/src/github.com/DataDog/cilium/hubble'

Difference is that the two separate -ldflags args are now combined as a single one.

operator/Makefile:

  • Remove reference to non defined GOCLEAN variable and replace $(GO) clean with $(GO_CLEAN)
  • Update/Add .DEFAULT_GOAL and .PHONY targets

Note: the GO_BUILD variable from Makefile.defs already automatically wires up the go build flags and ldflags, simplifying usage.

@HadrienPatte HadrienPatte requested review from a team as code owners February 18, 2025 15:51
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 18, 2025
@github-actions github-actions bot added hubble-cli PRs or GitHub issues related with hubble-cli cilium-cli This PR contains changes related with cilium-cli kind/community-contribution This was a contribution made by a community member. labels Feb 18, 2025
@HadrienPatte HadrienPatte marked this pull request as draft February 18, 2025 17:18
@HadrienPatte HadrienPatte marked this pull request as ready for review February 18, 2025 18:33
@kaworu kaworu added release-note/misc This PR makes changes that have no direct user impact. sig/hubble area/build Anything to do with the build, more general than area/CI labels Feb 21, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 21, 2025
@kaworu kaworu removed the sig/hubble label Feb 21, 2025
Copy link
Member

@kaworu kaworu left a 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.

@kaworu kaworu added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Mar 3, 2025
@HadrienPatte HadrienPatte force-pushed the makefile branch 2 times, most recently from 3596eb6 to 4adde52 Compare March 3, 2025 14:45
@HadrienPatte HadrienPatte requested review from kaworu and joamaki March 3, 2025 14:49
@joestringer
Copy link
Member

joestringer commented Mar 14, 2025

@kaworu @joamaki could you take a fresh look?

I strongly suspect this will also need another rebase due to changes that went into the tree in the last couple of weeks, otherwise CI will not pass.

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>
@joestringer
Copy link
Member

/test

@kaworu kaworu removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Mar 21, 2025
@joestringer joestringer enabled auto-merge March 24, 2025 14:10
@joestringer
Copy link
Member

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
Copy link
Member

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.

@joestringer joestringer disabled auto-merge March 24, 2025 14:21
@joestringer joestringer merged commit db3445d into cilium:main Mar 24, 2025
67 checks passed
@joestringer
Copy link
Member

Thanks for the cleanup @HadrienPatte !

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 than area/CI cilium-cli This PR contains changes related with cilium-cli hubble-cli PRs or GitHub issues related with hubble-cli kind/community-contribution This was a contribution made by a community member. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants