https://bugzilla.redhat.com/show_bug.cgi?id=2269411 --- Comment #8 from Fabio Valentini <decathorpe@xxxxxxxxx> --- 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. -- 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=2269411 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202269411%23c8 -- _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx 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/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
