https://bugzilla.redhat.com/show_bug.cgi?id=2269411
--- Comment #12 from Daniel Mellado <dmellado(a)redhat.com> ---
(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=rep...