iovisor / bcc

BCC - Tools for BPF-based Linux IO analysis, networking, monitoring, and more
Apache License 2.0
20.36k stars 3.86k forks source link

Narrow libbcc dependency from llvm-dev to just libllvm #4997

Closed bobrik closed 4 months ago

bobrik commented 4 months ago

It works with list libllvm, so no need to pull in the whole thing, which is 600MiB worth of packages.

bobrik commented 4 months ago

@shunghsiyu, what exactly broke for you prior to #4921?

shunghsiyu commented 4 months ago

It works with list libllvm, so no need to pull in the whole thing, which is 600MiB worth of packages. @shunghsiyu, what exactly broke for you prior to #4921?

The problem for me is that using bcc header files (packaged as bcc-devel on RPM systems) requires llvm-devel to be on installed as well since 6f11bf7e2806658c4bd69415b921cedf85d9ebfe, otherwise compilation will fail. This is not a theoretical issue but an actual one that we saw with Sysinternals/ProcMon-for-Linux.

debian/control does not have a separate package to host the bcc header files so I ended up placing such requirement on the main bcc package, which is admittedly not ideal.

shunghsiyu commented 4 months ago

From a packaging point of view I'd much prefer we revert 6f11bf7e2806658c4bd69415b921cedf85d9ebfe to keep the dependency at a minimum.

That said I only know two users of bcc-devel: one is Sysinternals/ProcMon-for-Linux mentioned above, and the other is bpftrace/bpftrace. bpftrace already pulls llvm-devel itself anyway, while ProcMon-for-Linux doesn't get packaged everywhere; so this is perhaps a bit of bikeshedding.

@chantra what do you think?

chantra commented 4 months ago

On Sun, May 19, 2024 at 11:02 PM Shung-Hsi Yu @.***> wrote:

From a packaging point of view I'd much prefer we revert 6f11bf7 https://github.com/iovisor/bcc/commit/6f11bf7e2806658c4bd69415b921cedf85d9ebfe to keep the dependency at a minimum.

For packaging, this should only be a build dependency right: https://www.debian.org/doc/debian-policy/ch-relationships.html ? The package itself has a pretty minimal footprint (26kb on Ubuntu per https://packages.ubuntu.com/noble/llvm-dev). Probably this pulls more deps, but only on the build machine.

Quickly checking the issue, I think moving llvm-dev to a “Build-Depends” would solve the problem for machines where bcc gets installed.

Would that be a good enough middle ground?

That said I only know two users of bcc-devel: one is

Sysinternals/ProcMon-for-Linux https://github.com/Sysinternals/ProcMon-for-Linux mentioned above, and the other is bpftrace/bpftrace https://github.com/bpftrace/bpftrace. bpftrace already pulls llvm-devel anyway, while ProcMon-for-Linux doesn't get packaged everywhere; so this is perhaps a bit of bikeshedding.

@chantra https://github.com/chantra what do you think?

— Reply to this email directly, view it on GitHub https://github.com/iovisor/bcc/pull/4997#issuecomment-2119727600, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAF77DROQGITXS5HX7OYKTZDGGWXAVCNFSM6AAAAABHPIRQ7CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJZG4ZDONRQGA . You are receiving this because you were mentioned.Message ID: @.***>

chantra commented 4 months ago

If bcc-devel requires llvm-devel, I suppose the problem is because we pull llvm-config.h in bcc_module.h which is public? Moving llvm-config.h to private only headers should do the job.

Can you list the public headers and we can work on moving this include out of the way.

shunghsiyu commented 4 months ago

For packaging, this should only be a build dependency right

Not sure about Debian, but on the RPM side, even though llvm-devel is a build-time dependency for ProcMon-for-Linux, it should be listed as a runtime dependency for bcc-devel to get it transitively installed; otherwise llvm-devel do not get installed along with bcc-devel.

The follow graph depicts the dependency chain:

ProMon (build-time dependency with "BuildRequires: bcc-devel") --> bcc-devel (runtime dependency with "Requires: llvm-devel") --> llvm-devel
shunghsiyu commented 4 months ago

If bcc-devel requires llvm-devel, I suppose the problem is because we pull llvm-config.h in bcc_module.h which is public? Moving llvm-config.h to private only headers should do the job.

I think that will work

Can you list the public headers and we can work on moving this include out of the way.

The public headers I founds are:

So bpf_module.h should be the one that need modification.

chantra commented 4 months ago

5018 should fix this issue. @shunghsiyu could you confirm the package change are fine? cc @yonghong-song