https://bugzilla.redhat.com/show_bug.cgi?id=2269411 --- Comment #12 from Daniel Mellado <dmellado@xxxxxxxxxx> --- (In reply to Fabio Valentini from comment #8) > 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). > Thanks, this has been quite the pain. bpfman, for the workspace should be only Apache-2.0. We've modified the specfile to address this and the other licensing issues. > ============================================================ > > 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. We've gotten rid of the earlier version and now ship only ring v0.17 on. > > ============================================================ > > 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)". > Fixed in latest specfile, thanks! > ============================================================ > > 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). > Fixed in latest specfile > ============================================================ > > 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. Checked and in fact it was a false positive, thanks a lot for the heads-up as it would've been giving us quite the headache -- 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%23c12 -- _______________________________________________ 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
