https://bugzilla.redhat.com/show_bug.cgi?id=2281565
--- Comment #12 from Fabio Valentini <[email protected]> --- (In reply to Jose Fernandez from comment #11) > > There are some BuildRequires and Requires that don't look like they are > > actually necessary: > > > > """ > > BuildRequires: elfutils-libelf-devel > > BuildRequires: zlib-devel > > BuildRequires: clang > > > > Requires: elfutils-libelf > > Requires: zlib > > """ > > > > Is there a specific reason why you added these? > > I'm under the impression that libelf and zlib are required at link time to > build bpftop because it pulls in libbpf-rs, which compiles libbpf. They are > also needed for bpftop to run since it loads a bpf program. From the libbpf > docs: > "libelf and zlib are internal dependencies of libbpf and thus are required > to link against and must be installed on the system for applications to > work." > https://docs.kernel.org/bpf/libbpf/libbpf_build.html If libbpf already pulls these in, then you don't need to depend on them. > > > > At least the Requires for elfutils-libelf and zlib are unnecessary. Both > > libraries are dynamically linked, and dependencies on them are generated by > > RPM automatically. > > How does RPM detect that they are dependencies? From ELF metadata. The bpftop binary installed by this package links against libz.so.1 and libelf.so.1. RPM reads this metadata from all executable binaries in the package and automatically generates dependencies on packages that provide these libraries. So you should not specify them manually in addition to it, since it is redundant. > > > > The BuildRequires on elfutils-libelf-devel, zlib-devel and clang might be > > leftovers from building with vendored sources? > > > > Clang is needed for libbpf-rs to compile the bpf program embedded in bpftop. > libbpf-cargo handles the bpf compilation when you run `cargo build`, but it > relies on clang. If you're only using libbpf-cargo here, and libbpf-cargo relies on clang internally, then libbpf-cargo should pull in clang. > > The Packages for Rust crates > > should already pull in zlib-devel etc. (and clang for bindgen) as needed. > > My assumption is that it would be better to be explicit about the > dependencies needed and not to rely on transitive deps from the crates. I > assumed that it doesn't hurt. But happy to follow what you think is idea. In general, the rule of thumb is to only explicitly depend on things that you need *directly*. In this case, the package for libbpf-cargo should probably add a dependency on clang, if it's not working without it? > > Other than that, the package looks good to me! > > Thanks! I will wait to hear back and adjust accordingly. > > > > > PS: The spec file you added here: > > https://github.com/Netflix/bpftop/commit/a737971 is only compatible with > > recent versions of Fedora and RHEL 9 + EPEL, but will not work for building > > a package on any other RPM-based linux distribution. So I'm not sure how > > much benefit there is in keeping it in the upstream github repo. > > I can remove it. I wanted to put it there while I iterated on the spec. You don't need to remove it. I just wanted to let you know that keeping a separate copy of it might be of limited value long-term. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component https://bugzilla.redhat.com/show_bug.cgi?id=2281565 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202281565%23c12 -- _______________________________________________ package-review mailing list -- [email protected] To unsubscribe send an email to [email protected] Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/[email protected] Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
