Spec URL: https://dmellado.fedorapeople.org/bpfman/bpfman.spec SRPM URL: https://dmellado.fedorapeople.org/bpfman/bpfman-0.4.0-0.1.20240313gita11dd61.fc39.src.rpm Description: bpfman operates as an eBPF manager, focusing on simplifying the deployment and administration of eBPF programs. Fedora Account System Username: dmellado
Copr build: https://copr.fedorainfracloud.org/coprs/build/7156063 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2269411-bpfman/fedora-rawhide-x86_64/07156063-bpfman/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
[fedora-review-service-build]
Created attachment 2021529 [details] The .spec file difference from Copr build 7156063 to 7156145
Copr build: https://copr.fedorainfracloud.org/coprs/build/7156145 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2269411-bpfman/fedora-rawhide-x86_64/07156145-bpfman/fedora-review/review.txt Found issues: - No gcc, gcc-c++ or clang found in BuildRequires Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/ Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
coming from https://bugzilla.redhat.com/show_bug.cgi?id=2257948 in order to take over the review request. This new build includes https://github.com/bpfman/bpfman/commit/a34947384c29dc9068ce4063abca9ce2b4e8e0f5, which switches rusttls-tls to native-tls. This adds a compile-time dependency on having openssl libraries/headers available but sorts out the issues commented about cryptographic implementations as it now relies on the ones included in the system.
*** Bug 2257948 has been marked as a duplicate of this bug. ***
Thanks for the update - I will continue the review here ASAP.
I'm sorry this took a bit. I needed to wait until the stars aligned and fedora-review didn't crash on me while processing this package. Package looks pretty good in general, with some exceptions that should not be too hard to fix: ============================================================ 1. Wrong and / or incomplete license metadata: There are a few bundled crates that don't specify a license in SPDX expression in their Cargo.toml metadata. There's some crates with missing license metadata in the genreated LICENSE.dependencies. In Fedora, we fix this for packaged crates by always enforcing that metadata to be present and accurate (for our standards). You might need to apply some patches to the vendored sources as well: : integration-test v0.1.0 : integration-test-macros v0.1.0 (proc-macro) : ring v0.16.20 : ring v0.17.8 : xtask v0.1.0 The first two and xtask seem to be nested crates from bpfman since they don't match this crate: https://crates.io/crates/integration-test I'm not sure why these three show up in the LICENSE.dependencies file at all ... are they somehow compiled into the binary despite looking like test-only or development-only dependencies? Or maybe you should run the `%cargo_license` and `%cargo_license_summary` macros only in the `bpfman` subdirectory, since that's the workspace member that contains the shipped executable. As for ring, the upstream project refuses to specify a license in its metadata for weird reasons. All people who have audited the "ring" source code seem to agree that the correct expression would be "ISC AND MIT AND OpenSSL". To get this correctly taken into account by the macros, you would need to patch the Cargo.toml files for ring v0.16 and ring v0.17. Similarly, some crates bundle Unicode data but don't declare this in their license metadata (bstr and regex-syntax). At least this case does not affect generated license summary, because one of the crates in the dependency tree already has "Unicode-DFS-2016" in its license expression. ============================================================ 2. Mismatch between metadata license and license files in bpfman itself: The Cargo.toml file for bpfman claims the project is licensed Apache-2.0. However, there are additional license texts included in the source archive: LICENSE-BSD2 and LICENSE-GPL2. I was not able to determine whether (if at all) which parts of bpfman these licenses apply to. The only files with different license headers I found were: - proto/google/protobuf/descriptor.proto: BSD-3-Clause header - proto/google/protobuf/timestamp.proto: BSD-3-Clause header - proto/google/protobuf/wrappers.proto: BSD-3-Clause header - bpf/xdp_dispatcher_v1.bpf.c: GPL-2.0-only header - bpf/xdp_dispatcher_v2.bpf.c: GPL-2.0-only header There don't seem to be any BSD-2-Clause licensed files in the sources, only BSD-3-Clause. Is the inclusion of the BSD-2-Clause license text a mistake and should the project include a BSD-3-Clause license text instead? Also, I'm not sure if that would be necessary, since the relevant files already contain a whole copy of the license text in their header. I can't really tell whether those files are compiled into the final executable of bpfman or if they're used for different purposes. If they *do* end up in the compiled binary and shipped in the "bpfman" binary package, their licenses must be taken into account. At least the SourceLicense tag should be "Apache-2.0 AND BSD-3-Clause AND GPL-2.0-only" in that case (remove items as appropriate). ============================================================ 3. Dependencies on two different versions of the "ring" crate (the older one with limited architecture support): : ring v0.16.20 : ring v0.17.8 This is a big problem in itself (I think "ring" uses symbol prefixes for C and Assembly implementations to avoid linker problems). However, you might want to ensure that the package only pulls in *one* of these versions, and preferably the newer one (v0.17). The v0.16 branch of "ring" does not support ppc64le and s390x, and so you will not be able to build bpfman for these architectures unless you avoid ring v0.16 in favor of ring v0.17. ============================================================ 4. Statically linking some C dependencies instead of dynamically linking system libraries: This is a problem because some "bindings" (-sys) crates default to building and statically linking a vendored copy of the wrapped C library. In Fedora, we address this by patching the crate sources to unconditionally link dynamically. You might need to apply similar patches to some vendored crates. It looks like this currently only affects the "libz-sys" crate, which bundles both zlib and zlib-ng and falls back to building them from source if the zlib pkg-config file cannot be found. As far as I can tell, bpfman does not link to libz.so, which is probably *not* what you want. Looking at the build script for libz-sys, you might be able to fix this by adding BuildRequires on "pkgconfig(zlib)". ============================================================ 5. No dependency on a C compiler? Some of this crate's dependencies require a working C compiler to be present (notably, "ring" and other crates that contain C and / or Assembly). I would recommend to add "BuildRequires: gcc" explicitly. ============================================================ 6. Accidentally skipping all tests: The way you've split the "--skip" arguments for "%cargo_test" across multiple lines interferes with RPM's macro expansion. Using escaped newlines accidentally causes *no* tests to be run *at all*. I'm not sure why, but it's a known problem. Even if long lines like it are ugly, you will need to keep all arguments for %cargo_test on a single line for them to work correctly (and not accidentally disabling *all* tests, not only the ones explicitly mentioned). ============================================================ 7. Possible false positive OpenSSL warning from rpmlint: bpfman.x86_64: W: crypto-policy-non-compliance-openssl /usr/sbin/bpfman SSL_CTX_set_cipher_list As far as I can tell, rpmlint is only checking for the presence of the symbol, not whether it's actually called in the code, so this is, as far as I can tell, a false positive. I've seen this in other packages too, and I was never able to find any code path that actually hit that function from OpenSSL. But it would be good to check.