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.
Hi @decathorpe, I think we've addressed all the points you comment there. I'll be replying in detail about them though ;)
Spec URL: https://dmellado.fedorapeople.org/bpfman/bpfman.spec SRPM URL: https://dmellado.fedorapeople.org/bpfman/bpfman-0.4.1-0.1.20240515git8b253cf5.fc40.src.rpm Description: bpfman operates as an eBPF manager, focusing on simplifying the deployment and administration of eBPF programs. Fedora Account System Username: dmellado
(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
Copr build: https://copr.fedorainfracloud.org/coprs/build/7467146 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2269411-bpfman/fedora-rawhide-x86_64/07467146-bpfman/fedora-review/review.txt Found issues: - Systemd service file(s) in bpfman Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_scriptlets 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.
Copr build: https://copr.fedorainfracloud.org/coprs/build/7467145 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2269411-bpfman/fedora-rawhide-x86_64/07467145-bpfman/fedora-review/review.txt Found issues: - Systemd service file(s) in bpfman Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_scriptlets 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.
Created attachment 2034227 [details] The .spec file difference from Copr build 7467146 to 7469312
Copr build: https://copr.fedorainfracloud.org/coprs/build/7469312 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2269411-bpfman/fedora-rawhide-x86_64/07469312-bpfman/fedora-review/review.txt Please take a look if any issues were found. --- 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.
Sorry for the delay. Package looks pretty good, with some remaining and / or new issues: 1. The license tag in the spec file is just "Apache-2.0". This MUST reflect all statically linked crates, i.e. the summary printed by %cargo_license_summary (which is itself a summary of the contents of the LICENSE.dependencies file). 2. The license breakdown still contains crates without license information: - : ring v0.17.8 - : sigstore_protobuf_specs v0.1.0-rc.2 I suggest that you patch the vendored Cargo.toml for "ring" to remove the "license-file" metadata and add `license = "ISC AND MIT AND OpenSSL"` instead. For sigstore_protobuf_specs, it looks like you're vendoring a *very old* version that still had non-standard license. The latest versions (published within the last two months) all specify "Apache-2.0" as their license. for reference: https://crates.io/crates/sigstore_protobuf_specs/versions 3. "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." It's still not clear to me (even after your changes) why the project contains license texts for BSD-2-Clause and GPL-2.0. Are you implying these licenses only apply to files that don't end up in the built package? Have you verified this? 4. You are still bundling a version of the fiat-crypto crate. This crate contains implementations of elliptic-curve cryptography that is *NOT* approved to be shipped by Fedora *in any form* (i.e. also not as source code). You will need to patch out any references to the p434 curve *before* compressing the vendor tarball. You can take the patch from the Fedora package for the crate (rust-fiat-crypto). see also: https://lists.fedoraproject.org/archives/list/legal%40lists.fedoraproject.org/thread/FBZU2X7ZKTK2BVZKBHFUCI44SMY4UQCE/
It's been over three months since my last round of review - Are you still intending to work on this package?
(In reply to Fabio Valentini from comment #19) > It's been over three months since my last round of review - Are you still > intending to work on this package? Hi Fabio, yes! I do have an updated version and will submit, however I need to figure out what we commented about handling bundled deps with rust2rpm.
(In reply to Fabio Valentini from comment #18) > Sorry for the delay. Package looks pretty good, with some remaining and / or > new issues: > > 1. The license tag in the spec file is just "Apache-2.0". > This MUST reflect all statically linked crates, i.e. the summary printed by > %cargo_license_summary (which is itself a summary of the contents of the > LICENSE.dependencies file). > > 2. The license breakdown still contains crates without license information: > > - : ring v0.17.8 > - : sigstore_protobuf_specs v0.1.0-rc.2 > > I suggest that you patch the vendored Cargo.toml for "ring" to remove the > "license-file" metadata and add `license = "ISC AND MIT AND OpenSSL"` > instead. > > For sigstore_protobuf_specs, it looks like you're vendoring a *very old* > version that still had non-standard license. > The latest versions (published within the last two months) all specify > "Apache-2.0" as their license. > > for reference: https://crates.io/crates/sigstore_protobuf_specs/versions > > 3. "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." > > It's still not clear to me (even after your changes) why the project > contains license texts for BSD-2-Clause and GPL-2.0. > Are you implying these licenses only apply to files that don't end up in the > built package? Have you verified this? > > 4. You are still bundling a version of the fiat-crypto crate. > > This crate contains implementations of elliptic-curve cryptography that is > *NOT* approved to be shipped by Fedora *in any form* (i.e. also not as > source code). > > You will need to patch out any references to the p434 curve *before* > compressing the vendor tarball. > You can take the patch from the Fedora package for the crate > (rust-fiat-crypto). > > see also: > https://lists.fedoraproject.org/archives/list/legal%40lists.fedoraproject. > org/thread/FBZU2X7ZKTK2BVZKBHFUCI44SMY4UQCE/ Hi Fabio, thanks for your comments. I've fetched the script in https://koji.fedoraproject.org/koji/fileinfo?rpmID=39412032&filename=0001-remove-references-to-code-related-to-the-p434-curve.patch and applied it the fiat-crypto sources. As we commented over Matrix, I've also removed the commit id from the specfile and now I do mention it directly over the tag. My steps over here are cargo vendor --versioned-dirs <mangle fiat-crypto here> (patch p1 < ) tar -Jcvf vendor/ ../tarball.xz But now, using the updated specfile and vendor here. Spec URL: https://dmellado.fedorapeople.org/bpfman/bpfman.spec SRPM URL: https://dmellado.fedorapeople.org/bpfman/bpfman-0.5.1-vendor.tar.xz I hit an issue about the offline mode (that IIUC it shouldn't be triggered as we're vendoring) https://paste.opendev.org/show/bW7HO1ssP3Xq0M8OCcAF/ Mind taking a look? Thanks!
> %cargo_license_summary > %cargo_license Not sure why you copied these macros to the %prep scriptlet? They do nothing there, and they're duplicated in %build later (where they belong). I'm not sure that this is the cause of the problem, but it's not helping. > License: Apache-2.0 This is also not correct, it needs to be adapted according to the output of the %cargo_license_summary macro. If SourceLicense and License were actually identical, you'd only need to specify the latter. > Spec URL: https://dmellado.fedorapeople.org/bpfman/bpfman.spec > SRPM URL: https://dmellado.fedorapeople.org/bpfman/bpfman-0.5.1-vendor.tar.xz Looks like you copied the vendor tarball instead of the SRPM here?
The problem with "offline mode" seems to be caused by git snapshot dependencies that cargo seems to try to "update" even though they're already vendored. "cargo vendor" doesn't handle git dependencies well ... and I don't currently have a good workaround for it. The people packaging the COSMIC desktop environment have had similar problems, I would recommend reaching out to them and asking how they resolved that problem. You might need to use "%cargo_prep -N" instead of "%cargo_prep -v vendor" and cat >> your configuration for vendored sources manually.
Just wanted to sync the bugzilla with the latest comments over matrix. I'm currently blocked on the `aya` release. I'm pushing for them to cut a new release that includes the necessary PR [aya#921](https://github.com/aya-rs/aya/pull/921) to remove the Git dependency from the `bpfman` Cargo setup ([bpfman Cargo.toml#L103-L105](https://github.com/bpfman/bpfman/blob/main/Cargo.toml#L103-L105)). The current version I'm working with is tagged as [v0.5.1](https://github.com/bpfman/bpfman/tree/v0.5.1). I've also attempted to run `cargo package` on the `main` branch of `aya`, but it's failing on certain dependencies. I'm still investigating how they manage this. Would it make sense to ship an older version of `bpfman` for Fedora in the meantime? I could potentially use `v0.5.0` as a fallback, though I'm trying to avoid this, as we'll encounter the same issue when updating. Additionally, I'm filing an issue with `rust2rpm` to handle Git dependencies in Cargo, though this might be more of a Cargo limitation than a `rust2rpm` issue.
Spec URL: https://dmellado.fedorapeople.org/bpfman/bpfman.spec SRPM URL: https://dmellado.fedorapeople.org/bpfman/bpfman-0.5.2-1.fc40.src.rpm Description: bpfman operates as an eBPF manager, focusing on simplifying the deployment and administration of eBPF programs. Fedora Account System Username: dmellado
Thanks for the update. I'll take a look as soon as I have some time (probably tomorrow). [fedora-review-service-build]
Created attachment 2052367 [details] The .spec file difference from Copr build 7469312 to 8148854
Copr build: https://copr.fedorainfracloud.org/coprs/build/8148854 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2269411-bpfman/fedora-rawhide-x86_64/08148854-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.
Adding # Remove the executable bit from lib.rs as it affects brp_mange_shebangs chmod -x ./vendor/typed-path/src/lib.rs for F41/Rawhide
Created attachment 2052491 [details] The .spec file difference from Copr build 8148854 to 8150506
Copr build: https://copr.fedorainfracloud.org/coprs/build/8150506 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2269411-bpfman/fedora-rawhide-x86_64/08150506-bpfman/fedora-review/review.txt Please take a look if any issues were found. --- 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.
bpfman 0.53 and 0.54 got released. I'll submit a new updated build
Spec URL: https://dmellado.fedorapeople.org/bpfman/bpfman.spec SRPM URL: https://dmellado.fedorapeople.org/bpfman/bpfman-0.5.4-1.fc40.src.rpm Description: bpfman operates as an eBPF manager, focusing on simplifying the deployment and administration of eBPF programs. Fedora Account System Username: dmellado
[fedora-review-service-build] *pokes bot*
Created attachment 2055533 [details] The .spec file difference from Copr build 8150506 to 8208198
Copr build: https://copr.fedorainfracloud.org/coprs/build/8208198 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2269411-bpfman/fedora-rawhide-x86_64/08208198-bpfman/fedora-review/review.txt Please take a look if any issues were found. --- 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.
Doing another review round: 1. > SourceLicense: Apache-2.0 > License: Apache-2.0 > # LICENSE.dependencies contains a full license breakdown Both SourceLicense and License are wrong here. The SourceLicense tag doesn't apply here (it would need to cover the vendor tarball), so it's better to not specify it at all. The License tag doesn't account for statically linked dependencies, i.e. the output of the `%{cargo_license_summary}` macro. 2. > BuildRequires: openssl-devel > BuildRequires: pkgconfig(zlib) > BuildRequires: gcc It would be great to get these documented, something like this: """ # dependency for the bundled openssl-sys crate BuildRequires: openssl-devel # dependency for the bundled libz-sys crate BuildRequires: pkgconfig(zlib) # dependency for the bundled cc crate BuildRequires: gcc """ 3. > # Remove references to p434 curve in fiat-crypto > sed -i '/^pub mod p434_64;/d' vendor/fiat-crypto-0.2.9/src/lib.rs The source code is still in the vendor tarball. Fedora is not allowed to distribute that. You will need to remove the file from the vendored fiat-crypto sources and recompress the vendor tarball. 4. > # Skip image_pull_* tests as require Internet to pull images from a registry > %cargo_test -- -- --skip oci_utils::image_manager::tests::image_pull_failure --skip oci_utils::image_manager::tests::image_pull_and_bytecode_verify --skip oci_utils::image_manager::tests::private_image_pull_and_bytecode_verify --skip oci_utils::image_manager::tests::image_pull_policy_never_failure If all you need to do is skip all tests that match "image_pull_", then this is equivalent: > %cargo_test -- -- --skip image_pull_ Since cargo does substring matching on test names by default (to use exact string matches, the "--exact" flag needs to be passed).
(In reply to Fabio Valentini from comment #39) > Doing another review round: > > 1. > > > SourceLicense: Apache-2.0 > > License: Apache-2.0 > > # LICENSE.dependencies contains a full license breakdown > > Both SourceLicense and License are wrong here. > > The SourceLicense tag doesn't apply here (it would need to cover the vendor > tarball), so it's better to not specify it at all. > The License tag doesn't account for statically linked dependencies, i.e. the > output of the `%{cargo_license_summary}` macro. > > 2. > > > BuildRequires: openssl-devel > > BuildRequires: pkgconfig(zlib) > > BuildRequires: gcc > > It would be great to get these documented, something like this: > > """ > # dependency for the bundled openssl-sys crate > BuildRequires: openssl-devel > # dependency for the bundled libz-sys crate > BuildRequires: pkgconfig(zlib) > # dependency for the bundled cc crate > BuildRequires: gcc > """ > > 3. > > > # Remove references to p434 curve in fiat-crypto > > sed -i '/^pub mod p434_64;/d' vendor/fiat-crypto-0.2.9/src/lib.rs > > The source code is still in the vendor tarball. Fedora is not allowed to > distribute that. You will need to remove the file from the vendored > fiat-crypto sources and recompress the vendor tarball. > > 4. > > > # Skip image_pull_* tests as require Internet to pull images from a registry > > %cargo_test -- -- --skip oci_utils::image_manager::tests::image_pull_failure --skip oci_utils::image_manager::tests::image_pull_and_bytecode_verify --skip oci_utils::image_manager::tests::private_image_pull_and_bytecode_verify --skip oci_utils::image_manager::tests::image_pull_policy_never_failure > > If all you need to do is skip all tests that match "image_pull_", then this > is equivalent: > > > %cargo_test -- -- --skip image_pull_ > > Since cargo does substring matching on test names by default (to use exact > string matches, the "--exact" flag needs to be passed). Thanks for the detailed review! I addressed your points and added a few more comments in the specfile for extra clarification.
Copr build: https://copr.fedorainfracloud.org/coprs/build/8209767 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2269411-bpfman/fedora-rawhide-x86_64/08209767-bpfman/fedora-review/review.txt Please take a look if any issues were found. --- 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.
It looks like you uploaded the wrong (or old) SRPM file?
(In reply to Fabio Valentini from comment #43) > It looks like you uploaded the wrong (or old) SRPM file? Yeah, I pasted the wrong link over there, let me update it, sigh!
Spec URL: https://dmellado.fedorapeople.org/bpfman/bpfman.spec SRPM URL: https://dmellado.fedorapeople.org/bpfman/bpfman-0.5.4-1.fc41.src.rpm Description: bpfman operates as an eBPF manager, focusing on simplifying the deployment and administration of eBPF programs. Fedora Account System Username: dmellado
Created attachment 2056185 [details] The .spec file difference from Copr build 8209767 to 8225284
Copr build: https://copr.fedorainfracloud.org/coprs/build/8225284 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2269411-bpfman/fedora-rawhide-x86_64/08225284-bpfman/fedora-review/review.txt Found issues: - Not a valid SPDX expression 'Apache-2.0 \ AND BSD-3-Clause \ AND ISC \ AND MIT \ AND MPL-2.0 \ AND OpenSSL \ AND Unicode-DFS-2016 \ AND Zlib \ AND (0BSD OR MIT OR Apache-2.0) \ AND (Apache-2.0 OR BSL-1.0) \ AND (Apache-2.0 OR ISC OR MIT) \ AND (Apache-2.0 OR MIT) \ AND (Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT) \ AND (BSD-2-Clause OR Apache-2.0 OR MIT) \ AND (MIT AND BSD-3-Clause) \ AND (MIT OR Apache-2.0 OR BSD-1-Clause) \ AND (MIT OR Apache-2.0 OR Zlib) \ AND (Unlicense OR MIT) \'. Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1 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.
license-validate didn't like the backslashes, I'm uploading a slightly modified version while keeping the licensing and adding some comments for clarity
Created attachment 2056277 [details] The .spec file difference from Copr build 8225284 to 8226023
Copr build: https://copr.fedorainfracloud.org/coprs/build/8226023 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2269411-bpfman/fedora-rawhide-x86_64/08226023-bpfman/fedora-review/review.txt Please take a look if any issues were found. --- 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.
Added some minor cleanup to the shrink macro
Created attachment 2056299 [details] The .spec file difference from Copr build 8226023 to 8226708
Copr build: https://copr.fedorainfracloud.org/coprs/build/8226708 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2269411-bpfman/fedora-rawhide-x86_64/08226708-bpfman/fedora-review/review.txt Please take a look if any issues were found. --- 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.
> # References to code related to the p434 curve have been removed from the vendored sources in Source1 I cannot confirm this. The file "fiat-crypto-0.2.9/src/p434_64.rs" is still present in the vendor tarball. Other than that, looks good to me now.
(In reply to Fabio Valentini from comment #56) > > # References to code related to the p434 curve have been removed from the vendored sources in Source1 > > I cannot confirm this. The file "fiat-crypto-0.2.9/src/p434_64.rs" is still > present in the vendor tarball. > > Other than that, looks good to me now. I see, I had thought that it would've been fine removing the references in 'src/lib.rs', I'll completely remove that file from the vendored sources as well and reupload it. Thanks!
Copr build: https://copr.fedorainfracloud.org/coprs/build/8229649 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2269411-bpfman/fedora-rawhide-x86_64/08229649-bpfman/fedora-review/review.txt Please take a look if any issues were found. --- 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.
(In reply to Daniel Mellado from comment #57) > (In reply to Fabio Valentini from comment #56) > > > # References to code related to the p434 curve have been removed from the vendored sources in Source1 > > > > I cannot confirm this. The file "fiat-crypto-0.2.9/src/p434_64.rs" is still > > present in the vendor tarball. > > > > Other than that, looks good to me now. > > I see, I had thought that it would've been fine removing the references in > 'src/lib.rs', I'll completely remove that file from the vendored sources as > well and reupload it. Thanks! No, the code must not be present in both the source archive and the package, pending legal review: https://lists.fedoraproject.org/archives/list/legal%40lists.fedoraproject.org/thread/FBZU2X7ZKTK2BVZKBHFUCI44SMY4UQCE/
Ok, final review: > Source1: https://dmellado.fedorapeople.org/bpfman/bpfman-%{version}-vendor.tar.xz This isn't really good. It's unclear how you created this tarball. You don't need to have it available for download at a URL, but you need to provide steps to reproduce its contents (either in the spec file as a comment, or in a separate script). > %autosetup -n bpfman-%{version} -p1 -a1 > # Source1 is vendored dependencies > tar -xf %{SOURCE1} -C vendor/ This is redundant? The -a1 flag for %autosetup in the first line should cause exactly the same thing to happen as the manual "tar" command in the third line. > %{_sbindir}/bpfman > %{_sbindir}/bpfman-ns > %{_sbindir}/bpfman-rpc Do these three really need to live in /usr/sbin? There's an ongoing (though incomplete) effort to merge contents of /usr/sbin to /usr/bin: https://fedoraproject.org/wiki/Changes/Unify_bin_and_sbin ================================================================================ There's also still the question of which licenses actually apply to bpfman itself. The project contains license files for Apache-2.0, BSD-2-Clause, and GPL-2.0. I just noticed that the .licenserc.yaml file seems to contain a breakdown of which files are covered by which license - These three files are listed as being GPL-2.0-only: - bpf/xdp_dispatcher_v1.bpf.c - bpf/xdp_dispatcher_v2.bpf.c - examples/go-xdp-counter/bpf/xdp_counter.c These three files are listed as being GPL-2.0-only OR BSD-2-Clause: - bpf/tc_dispatcher.bpf.c - examples/**/bpf/*.c - tests/**/*.bpf.c If any of these files contribute to the contents of the built package, then the license of these files needs to be manually taken into account (i.e. added to the list that's `%shrink`d into the License tag), and documented with a comment in the spec file. ================================================================================ Other than these things, the package looks good to me.
Created attachment 2060794 [details] The .spec file difference from Copr build 8229649 to 8333282
Copr build: https://copr.fedorainfracloud.org/coprs/build/8333282 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2269411-bpfman/fedora-rawhide-x86_64/08333282-bpfman/fedora-review/review.txt Please take a look if any issues were found. --- 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.
Thanks for the update! There's one last issue I see. You updated the spec file to install executables to %_bindir instead of %_sbindir, but the systemd service file still references /usr/sbin/bpfman. I suspect that you'll also need to patch the .service file for this change: -ExecStart=/usr/sbin/bpfman-rpc +ExecStart=/usr/bin/bpfman-rpc
Thanks! Yeah, totally forgot to update that script. Did that in the prep section and also added a comment, should be ok now ;)
Created attachment 2061686 [details] The .spec file difference from Copr build 8333282 to 8364126
Copr build: https://copr.fedorainfracloud.org/coprs/build/8364126 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2269411-bpfman/fedora-rawhide-x86_64/08364126-bpfman/fedora-review/review.txt Please take a look if any issues were found. --- 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.
Thank you - the change looks good to me. I'll just run fedora-review again to make sure I didn't miss anything, but I'll likely approve this package as-is later.
There are two rpmlint warnings that might be notable: > bpfman.x86_64: W: crypto-policy-non-compliance-openssl /usr/bin/bpfman SSL_CTX_set_cipher_list > bpfman.x86_64: W: crypto-policy-non-compliance-openssl /usr/bin/bpfman-rpc SSL_CTX_set_cipher_list However, I know that this is a false positive for any package that uses the "openssl" crate (which provides *bindings* for that function), even if it doesn't actually *call* this function. ============================================================ To summarize: I won't post the full review template here - all aspects of this package have already been thoroughly reviewed. - The package does not contain non-redistributable content. - It builds and installs correctly in rawhide. - Vendored dependencies are handled correctly. - The license tag reflects all content of the package *and* statically linked dependencies. - RPM scriptlets for the systemd service are present. Package APPROVED. ============================================================ One suggestion I have for the future: > Source1: https://dmellado.fedorapeople.org/bpfman/bpfman-%{version}-vendor.tar.xz You don't have to host the vendor.tar.xz file on a public server. That file will just be generated once and then uploaded to the Fedora lookaside cache (with "fedpkg new-sources"), where it will be immutable. Pointing it to a URL (that you own) is not necessary (and in fact, looks a bit strange, for this case). Just use the plain filename, no URL necessary: > Source1: bpfman-%{version}-vendor.tar.xz There's also a typo on line 84 ("fiile"), but that's a very cosmetic issue. :)
Of course I see one more thing after pushing the button: You might want to add """ %license LICENSE-GPL2 %license LICENSE-BSD2 """ to the %files list in addition to %license LICENSE-APACHE. Both are part of the license tag, so they should likely also be included as license texts.
The Pagure repository was created at https://src.fedoraproject.org/rpms/bpfman
FEDORA-2024-5814c2fd6d (bpfman-0.5.4-2.fc42) has been submitted as an update to Fedora 42. https://bodhi.fedoraproject.org/updates/FEDORA-2024-5814c2fd6d
FEDORA-2024-5814c2fd6d (bpfman-0.5.4-2.fc42) has been pushed to the Fedora 42 stable repository. If problem still persists, please make note of it in this bug report.