embedded: Delete support #2742
embedded: Delete support #2742
Conversation
danobi
force-pushed
the
rm_embedded
branch
6 times, most recently
from
7a8925c to
167fff4
Compare
danobi
added
do-not-merge
danobi
removed
the
do-not-merge
There was a problem hiding this comment.
This is an awesome cleanup!
Main README also contains mentions of build-static.sh (section Development), you'll want to remove that, too.
Also, I'd like to clarify what is the currently recommended development workflow. Previously, we had the bcc and libbpf submodules, build-libs.sh to build them locally, and we statically linked bpftrace against them. This made sure that we were always developing for a single BCC/libbpf version (the latest one) and didn't have to deal with supporting multiple versions that we did before.
Do we have something similar with nix? Can we document that somewhere?
Ack. Good catch.
The flake defines the exact libbpf and bcc version to use: https://github.com/iovisor/bpftrace/blob/e6e4318a2005992df121c6d25412c725e1cfb6d1/flake.nix#L24-L60 It downloads and builds from github. For people on bleeding edge distros (like me on arch linux), I don't expect the flake will be required. The distro libbpf + bcc should be new enough. But for folks on more stable distros, maybe we could recommend them use They are also welcome to install libbpf + bcc from source too. If that sounds reasonable I can document that in the developers.md doc |
danobi
force-pushed
the
rm_embedded
branch
2 times, most recently
from
40da90c to
a3473b9
Compare
|
I updated the README and removed all the references to |
There was a problem hiding this comment.
Couple of comments:
- Yesterday at the office hours, we discussed that the installation instructions for various distros in
INSTALL.mdare often outdated b/c some packages were renamed, etc. The original purpose of some Dockerfiles (e.g.Dockerfile.fedora30) was to simplify maintaining those instructions. While they're probably outdated, too, we plan to update them to serve their original purpose so I'm not sure that deleting them is the best approach. I think we should eventually end up with something likeDockerfile.fedora,Dockerfile.ubuntu,Dockerfile.alpine, etc., each for the latest version of the distro. - I may be missing something but I believe that this breaks our current process of building and pushing images to quay. Should we update
docs/release_process.mduntil we fix it?
Sounds good, would be nice to add it to |
Agreed, Dockerfiles were good documentation. However, in the current (now deleted) state, they reflected fairly old distro releases with quite a few manually built packages (like libcereal and in certain cases pahole). So they will require a rewrite. I've tacked #2756 onto #2626 . Mind if I take that on in a followup PR?
Yes, you are right. My plan was to upload the appimage as a workflow artifact (see https://docs.github.com/en/actions/managing-workflow-runs/downloading-workflow-artifacts?tool=webui). I don't think it's too difficult -- let me add that to this PR and update docs |
|
I cleaned up the commits a bit as well as addressing feedback |
There was a problem hiding this comment.
@danobi It seems like we'd be able to get a lot of the build simplifications by just removing the embedded build and leaving the STATIC_LINKING option alone. Would that make sense?
@tnovak When the USE_SYSTEM_BPF_BCC option goes away, it'll be in favour of always using the system libraries rather than the other way around, so you won't need to change anything for this part. (I think we could look into using CMake's FetchContent to get the best of both worlds here: use system libraries if installed, otherwise automatically vendor them)
|
Hey @danobi!
I'm not too familiar with AppImage, but a cursory search indicates that Android isn't supported out of the box; presumably because Android is just different enough from a typical Linux distribution that making it work would be non-trivial. For example, Android uses its own dynamic loader (installed as In general, there are only a few libraries that are guaranteed to be available.
Just |
Yeah, that's was I was thinking too. I wouldn't mind keeping the |
|
Btw, we have an office hours this week if anyone wants to drop in and chat about this. Should be links to the meeting info in the main readme |
|
As discussed during office hours last week, we will keep |
danobi
force-pushed
the
rm_embedded
branch
2 times, most recently
from
87ffd9d to
393cd96
Compare
|
Ready for another look. I'm fairly pleased with what we've arrived at |
This deletes all the infra for embedded builds as well as the infra for doing custom libbpf and bcc builds. We will be replacing this with the nix + the bpftrace appimage. Next commit we will delete embedded build support from the build system.
This deletes build support for embedded builds. Note we leave in support for semi-static builds. IOW: statically link against all dependencies except libc (b/c libc cannot be statically linked reliably).
This flag was used to link bpftrace against system installed libbpf and bcc. Now that we are controlling those dependencies with nix instead of submodules, we no longer need this flag.
It's good to have all the developer documentation in a single place.
This commit creates a new GHA job that builds and uploads the bpftrace appimage to the commit's build artifacts. This is intended to replace the previous quay.io images. Also update scripts/create-assets.md and release_process.md to use the appimage. Note we stop compressing the bpftrace binary alone -- the appimage is already zstd compressed.
Most of what changed was that you need to link against all the llvm backends that the distro originally built llvm with. So that's the LLVM_TARGETS_TO_BUILD bit. The other bit was more or less present in the previous embedded build logic -- the unlinking of transitive dependencies. llvm's infra seems to want to link against shared libraries even for static archives. Odd. But we can work around it.
This commit adds a simple CI job that just tests if bpftrace can build statically. No additional tests are run -- in the past we have not seen much benefit in embedded-build tests. It was, however, a massive headache to maintain. So this time we only build. Nix was not chosen to do static builds as the `pkgsStatic` ecosystem has some pretty annoying bugs. Basically pkgsStatic applies overlays to try and statically build all the packages it typically dynamically links for. While clever, there are bugs everywhere. So it wasn't worth the hassle. Alpine linux + docker seemed like a much easier option to maintain.
|
Sounds like this is final call then. I'll wait 48 hrs before merging. There will also probably be a while where we can do a clean revert. |
PR description
This builds on #2736 in that we will rely on the appimage to do for us
what the embedded build used to do. That is, to provide a static binary.
Supporting and maintaining the embedded builds was very difficult,
as the combinations between distros were essentially endless. This
led to a maze of difficult-to-understand-and-modify build and ci code.
Unifying on the nix build will provide both simplicity and reproducibility.
A farewell
But let us not go without saying how awesome the embedded binaries
were. They were extremely useful on countless occasions for myself alone.
As I'm sure they were for other people. But engineering is full of tradeoffs
and I think it's time to let the embedded builds ride off into the sunset.
Hopefully the appimage can live up to the legacy left behind by the static
build.
This closes #2741.
Checklist
man/adoc/bpftrace.adocand if needed indocs/reference_guide.mdCHANGELOG.md