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.

Sign up for GitHub

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

Jump to bottom

Makefile cleanups / improvements #1600

Merged
merged 2 commits into from
Oct 7, 2024
Merged

Makefile cleanups / improvements #1600

merged 2 commits into from
Oct 7, 2024

Conversation

michi-covalent
Copy link
Collaborator

2 commits:

  • drop building 32-bit release binaries
  • use hubble target to build release binaries

@chancez @michi-covalent
Drop building 32-bit release binaries
c3f663b 
Depending on the upstream Cilium packages you import, you may
transitively pull in the agent, which does not build on 32-bit
OSes due to the following error, which also breaks the Hubble build:

vendor/github.com/cilium/cilium/pkg/mountinfo/mountinfo_linux.go:56:27: invalid operation: fst.Type == mntType (mismatched types int32 and int64)

Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez @michi-covalent
Makefile: Use hubble target to build release binaries
298cf70 
Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@michi-covalent michi-covalent requested a review from a team as a code owner October 6, 2024 00:20
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label PR is blocked until the release note is set label Oct 6, 2024
@michi-covalent michi-covalent added the release-note/minor This PR introduces functionality that users may find relevant to operating Hubble. label Oct 6, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label PR is blocked until the release note is set label Oct 6, 2024
@rolinh
Copy link
Member

rolinh commented Oct 7, 2024

Drop building 32-bit release binaries

Depending on the upstream Cilium packages you import, you may
transitively pull in the agent, which does not build on 32-bit
OSes due to the following error, which also breaks the Hubble build:

vendor/github.com/cilium/cilium/pkg/mountinfo/mountinfo_linux.go:56:27: invalid operation: fst.Type == mntType (mismatched types int32 and int64)

It looks like this error was fixed in cilium/cilium@6f629af wasn't it? It's a bit unfortunate to drop support for 32-bit arm (I guess 386 is not so much used nowadays).

@michi-covalent
Copy link
Collaborator Author

yeah the particular issue mentioned in the commit message has already been fixed i think. i can go either way regarding 32-bit binaries. fwiw we stopped releasing 32-bit binaries for cilium-cli a while back 💭 cilium/cilium-cli#2136

rolinh
rolinh approved these changes Oct 7, 2024
View reviewed changes
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, let's uniformize then. I guess if some users really rely on 32-bit binaries, they'll complain about the removal.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 7, 2024
@michi-covalent michi-covalent merged commit 9672e2d into main Oct 7, 2024
6 checks passed
@michi-covalent michi-covalent deleted the pr/michi/chance branch October 7, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rolinh rolinh rolinh approved these changes

@chancez chancez chancez approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR introduces functionality that users may find relevant to operating Hubble.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@michi-covalent @rolinh @chancez