Spec URL: https://download.copr.fedorainfracloud.org/results/fmaurer/stgit/fedora-rawhide-x86_64/06230210-rust-bzip2-rs/rust-bzip2-rs.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/fmaurer/stgit/fedora-rawhide-x86_64/06230210-rust-bzip2-rs/rust-bzip2-rs-0.1.2-1.fc39.src.rpm Description: Pure Rust bzip2 decompressor. Fedora Account System Username: fmaurer copr build: https://copr.fedorainfracloud.org/coprs/fmaurer/stgit/build/6230210/ The package is mostly the output of rust2rpm, but with disabled tests because they would need criterion. rust-bzip2-rs is a dependency of stgit which I'd ultimately like to unretire. This is my first Fedora package and I still need a sponsor.
Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=104279267
Copr build: https://copr.fedorainfracloud.org/coprs/build/6235796 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2228632-rust-bzip2-rs/fedora-rawhide-x86_64/06235796-rust-bzip2-rs/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.
I've got two comments, one from a Rust specific point of view, one from a general packaging view: 1. The "criterion" dev-dependency is (almost exclusively) only used for building / running benchmarks. We don't compile / run benchmarks during package builds (this is strongly discouraged, just like running linters). So this dependency is actually unused, and can be removed/patched out from Cargo.toml (i.e. using "rust2rpm -sp"). Doing that should allow you to build the package with the test suite enabled (i.e. flip the "check" bcond at the top again). 2. It's always good to document some decisions, like a) why are tests disabled, or b) what does this patch do, etc. In this case, it would be good to document either "tests are disabled because of a missing dependency" or, after fixing 1., "the criterion dev-dependency is only used for benchmarks / unused and can be dropped".
Thank you for the feedback. (In reply to Fabio Valentini from comment #3) > 1. The "criterion" dev-dependency is (almost exclusively) only used for > building / running benchmarks. > We don't compile / run benchmarks during package builds (this is strongly > discouraged, just like running linters). > So this dependency is actually unused, and can be removed/patched out from > Cargo.toml (i.e. using "rust2rpm -sp"). Doing that should allow you to build > the package with the test suite enabled (i.e. flip the "check" bcond at the > top again). Makes sense, I'll give it a try. When I update the spec file, should I add a comment formatted the same way as comment #0 with the new spec file to kick of a new copr build? > 2. It's always good to document some decisions, like a) why are tests > disabled, or b) what does this patch do, etc. > In this case, it would be good to document either "tests are disabled > because of a missing dependency" or, after fixing 1., "the criterion > dev-dependency is only used for benchmarks / unused and can be dropped". Sure, is it preferred to have this in the spec files itself as comments or in the git history when actually committing the spec file?
Spec URL: https://download.copr.fedorainfracloud.org/results/fmaurer/stgit/fedora-rawhide-x86_64/06238896-rust-bzip2-rs/rust-bzip2-rs.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/fmaurer/stgit/fedora-rawhide-x86_64/06238896-rust-bzip2-rs/rust-bzip2-rs-0.1.2-3.fc39.src.rpm I updated the package to include a patch removing the benchmarking dependencies (criterion and bzip2) and enabled the tests. Also added a comment documenting what was patched out of Cargo.toml and why. Figured this was the way to go from reading other spec files and review requests.
Updated koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=104324051
Created attachment 1981522 [details] The .spec file difference from Copr build 6235796 to 6238913
Copr build: https://copr.fedorainfracloud.org/coprs/build/6238913 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2228632-rust-bzip2-rs/fedora-rawhide-x86_64/06238913-rust-bzip2-rs/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.
Yup, looks good. Package builds successfully and tests run. One problem remains - one of the subpackages that get build is not installable: Error: Problem: conflicting requests - nothing provides (crate(tinyvec/nightly_const_generics) >= 1.1.0 with crate(tinyvec/nightly_const_generics) < 2.0.0~) needed by rust-bzip2-rs+nightly-devel-0.1.2-3.fc39.noarch (try to add '--skip-broken' to skip uninstallable packages or '--nobest' to use not only best candidate packages) The subpackage for the "nightly" feature depends on other nightly-Rust-only features, which we usually disable in our packages (since we don't ship a nightly Rust compiler, they are usually useless). So I would recommend to also remove the "nightly" feature from the [features] table in Cargo.toml (in addition to the previous changes). Doing this with "rust2rpm -p" should result in the "rust-bzip2-rs+nightly-devel" subpackage (i.e. the "%package -n rust-%{crate}+nightly-devel" section) to no longer be generated. If you want to reproduce this error locally, you can use the "--postinstall" flag for mock, which attempts to install any built packages into the build chroot after a successful build. Other than that, the package looks good.
Spec URL: https://download.copr.fedorainfracloud.org/results/fmaurer/stgit/fedora-rawhide-x86_64/06239112-rust-bzip2-rs/rust-bzip2-rs.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/fmaurer/stgit/fedora-rawhide-x86_64/06239112-rust-bzip2-rs/rust-bzip2-rs-0.1.2-4.fc39.src.rpm Updated the package to remove the nightly feature and rpm. Thanks for the explanation and pointing out the "--postinstall" flag, that's really helpful. Updated koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=104325786
Created attachment 1981527 [details] The .spec file difference from Copr build 6238913 to 6239125
Copr build: https://copr.fedorainfracloud.org/coprs/build/6239125 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2228632-rust-bzip2-rs/fedora-rawhide-x86_64/06239125-rust-bzip2-rs/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.
Looks good to me, thanks for the update! === Package was generated with rust2rpm, simplifying the review. - package builds and installs without errors on rawhide - test suite is run and all unit tests pass - latest version of the crate is packaged - license matches upstream specification (MIT OR Apache-2.0) and is acceptable for Fedora - license files are included with %license in %files - package complies with Rust Packaging Guidelines Package APPROVED. === There's some post-import tasks that I usually recommend for Rust packages, but since this isn't going to be imported yet, I'll give those steps later. === Congratulations, you got your first approved package. Before getting sponsored into the packager group, I suggest that you take a look here: https://docs.fedoraproject.org/en-US/fesco/Packager_sponsor_policy/#requirements TL;DR: There's a few ways in which you can demonstrate that you understood the Packaging Guidelines. In my experience, making high-quality, non-binding reviews of other pending packages is is usually a good way to get started. If you decide to continue this way, it's always better to look at a few different packages, i.e. not do reviews for three very similar packages, but for example, one written in C using CMake/meson as build system, maybe one Python package, and one Go / Rust package - you get the idea. Once that's done, feel free to ping me with links to those reviews (or Pull Requests, etc.) - My offer to sponsor you and guide you through the process of submitting your first package still stands.
Hello @fmaurer, since this is your first Fedora package, you need to get sponsored by a package sponsor before it can be accepted. A sponsor is an experienced package maintainer who will guide you through the processes that you will follow and the tools that you will use as a future maintainer. A sponsor will also be there to answer your questions related to packaging. You can find all active sponsors here: https://docs.pagure.org/fedora-sponsors/ I created a sponsorship request for you: https://pagure.io/packager-sponsors/issue/617 Please take a look and make sure the information is correct. Thank you, and best of luck on your packaging journey. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service
FEDORA-2024-55a9a29b72 has been submitted as an update to Fedora 40. https://bodhi.fedoraproject.org/updates/FEDORA-2024-55a9a29b72
FEDORA-2024-55a9a29b72 has been pushed to the Fedora 40 stable repository. If problem still persists, please make note of it in this bug report.