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

bazel: use a sysroot for compilation #24669

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

rockwotj
Copy link
Contributor

@rockwotj rockwotj commented Jan 2, 2025

This patchset is two fold:

First and foremost we have a hermetic sysroot we compile against for all builds so now system dependencies should not leak into the build, and we can define them from a docker image. This means we can remove some dependencies from the install-deps.sh script.

Secondly, wire up a docker image that can run the broker using the ultra slim distroless series of containers. There is probably more we can do on the packaging front, but this is a good step. I moved the LD_LIBRARY_PATH script to a golang binary since bash is not installed on the distroless images. I'm also thinking that we could move completely to the base image that distroless provides (no shared libraries present), then copy in our shared libraries from our sysroot, but I'm punting on that for now.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.3.x
  • v24.2.x
  • v24.1.x

Release Notes

  • none

@rockwotj rockwotj force-pushed the bazel-image branch 3 times, most recently from 8b02ac6 to 4e111a9 Compare January 3, 2025 19:39
@rockwotj rockwotj marked this pull request as ready for review January 3, 2025 19:39
@rockwotj rockwotj force-pushed the bazel-image branch 2 times, most recently from c916487 to a589024 Compare January 3, 2025 19:46
Big picture:

* Use Distroless containers from Google to have a minimal base image
* Since we don't have bash in these containers change our
  LD_LIBRARY_PATH loader script to static Golang binaries
* Mimic the layout/functionality of our existing images

We don't package host shared libraries (glibc, libgcc) however, so we
still need a solution to package those up, ideally with a clang sysroot.
Provide a sysroot from an ubuntu jammy docker image to provide a
consistent set of headers, and system libraries for us to use to
build Redpanda. This way there should be less differences between
machines and we can remove a couple of required packages for
development.
@rockwotj rockwotj requested review from dotnwat and removed request for twmb, Deflaimun, r-vasquez and gene-redpanda January 3, 2025 20:12
dotnwat
dotnwat previously approved these changes Jan 3, 2025
@@ -270,6 +270,38 @@ crate.annotation(
gen_build_script = "off",
)

# ====================================
Copy link
Member

Choose a reason for hiding this comment

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

bazel: support packaging as docker image
Big picture:

just to clarify, this is the proposal for the docker image packaging that'll eventually be uploaded into docker hub and used by the community/docker-compose/etc...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be a breaking change to use this exact configuration. I think we could offer this specific version as a redpanda-slim variant, but I think we'll stick with the existing packaging story for the medium term. Eventually, we should move our existing setup into Bazel, but I don't see that happening for awhile. I actually wanted to do this docker thing so that I could run/update the Wasm tests to not use the nightly build but use the actual docker image at HEAD, and we can orchestrate everything with Bazel.

Copy link
Member

Choose a reason for hiding this comment

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

so that I could run/update the Wasm tests to not use the nightly build but use the actual docker image at HEAD, and we can orchestrate everything with Bazel.

got it. yeh i wasn't sure how it was going to be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I do think we should move to something slimmed down like this eventually. But I should be more clear about the intent of this for sure

Comment on lines +136 to +140
llvm.sysroot(
name = "llvm_19_toolchain",
label = "@x86_64_sysroot//:sysroot",
targets = ["linux-x86_64"],
)
Copy link
Member

Choose a reason for hiding this comment

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

dang this is very cool. does the sysroot provide isolation, or is it merely providing precedence over the default system root (e.g. to pick up other bits installed on the system from bazel/install-deps.sh).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've not researched this in depth. It's at the end of the day passing --sysroot to clang, but anecdotally I've believe it provides isolation because the first pass I did the build failed to find valgrind and xfs headers even tho I have those locally (I was only pulling in glibc and libgcc_s)

Copy link
Member

Choose a reason for hiding this comment

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

ahh, i see--the other deps aren't for the compiler--stuff like bison, ragel, etc..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well considering my build works locally but fails in CI I think there is still some leaking from the local system happening...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although the missing header is in the sysroot...

Screenshot 2025-01-03 at 3 34 20 PM

Copy link
Member

Choose a reason for hiding this comment

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

eek

libgcc-12-dev \
valgrind \
xfslibs-dev \
&& apt-get clean && rm -rf /var/lib/apt/lists/*
Copy link
Member

Choose a reason for hiding this comment

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

curious why we don't put the rest of the deps into this list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which deps? The ones from bazel/install-deps.sh? I believe the rest of those are for tools that we need bazel to invoke (ragel, etc) are there others that are compile time?

Copy link
Member

Choose a reason for hiding this comment

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

yep, i realized this when you mentioned above that sysroot was just passed to clang. i was thinking sysroot was a bazel thing, not a compiler toolchain thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants