Spec URL: https://siosm.fedorapeople.org/srpms/rust-git-absorb.spec SRPM URL: https://siosm.fedorapeople.org/srpms/rust-git-absorb-0.9.0-2.fc45.src.rpm Description: Git commit --fixup, but automatic Fedora Account System Username: siosm Previous review: https://bugzilla.redhat.com/show_bug.cgi?id=2174380 Main changes since the package was retired: - Update to 0.9.0 (latest release) - Use vendored dependencies
I've pushed this manifests to https://src.fedoraproject.org/fork/siosm/rpms/rust-git-absorb/tree/main-0.9.0
Taking this review. Initial feedback: 1. The URL for Source2 is outdated. The git repository no longer contains that file in the 0.9.0 tag. Looks like you need to download the .adoc source and process it into ROFF with asciidoc. 2. The package doesn't build (at least a local test build failed), with test failures: ``` failures: ---- tests::and_rebase_flag_with_rebase_options stdout ---- thread 'tests::and_rebase_flag_with_rebase_options' (3478) panicked at src/lib.rs:475:34: could not run git rebase: Os { code: 2, kind: NotFound, message: "No such file or directory" } note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ---- tests::and_rebase_flag stdout ---- thread 'tests::and_rebase_flag' (3477) panicked at src/lib.rs:475:34: could not run git rebase: Os { code: 2, kind: NotFound, message: "No such file or directory" } failures: tests::and_rebase_flag tests::and_rebase_flag_with_rebase_options test result: FAILED. 44 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s ``` 3. What's the reasoning behind using vendored dependencies? It's a "should not unless not possible to do it otherwise". If there are only a few missing / outdated dependencies in Fedora, we can look into resolving those before resorting to vendored dependencies. (Also, the cargo tooling support for building with vendored dependencies is really gnarly, so in general, for long-term maintenance, I would recommend against using vendored dependencies.)
Copr build: https://copr.fedorainfracloud.org/coprs/build/10218157 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2446933-rust-git-absorb/fedora-rawhide-x86_64/10218157-rust-git-absorb/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.
Thanks for the review. > 1. The URL for Source2 is outdated. > > The git repository no longer contains that file in the 0.9.0 tag. Looks like > you need to download the .adoc source and process it into ROFF with asciidoc. Indeed, I'll update that. > 2. The package doesn't build (at least a local test build failed), with test > failures: > > ``` > failures: > ---- tests::and_rebase_flag_with_rebase_options stdout ---- > thread 'tests::and_rebase_flag_with_rebase_options' (3478) panicked at > src/lib.rs:475:34: > could not run git rebase: Os { code: 2, kind: NotFound, message: "No such > file or directory" } > note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace > ---- tests::and_rebase_flag stdout ---- > thread 'tests::and_rebase_flag' (3477) panicked at src/lib.rs:475:34: > could not run git rebase: Os { code: 2, kind: NotFound, message: "No such > file or directory" } > failures: > tests::and_rebase_flag > tests::and_rebase_flag_with_rebase_options > test result: FAILED. 44 passed; 2 failed; 0 ignored; 0 measured; 0 filtered > out; finished in 0.01s > ``` Hum, the package was building for me locally. I'll try that again. > 3. What's the reasoning behind using vendored dependencies? > > It's a "should not unless not possible to do it otherwise". If there are > only a few missing / outdated dependencies in Fedora, we can look into > resolving those before resorting to vendored dependencies. (Also, the cargo > tooling support for building with vendored dependencies is really gnarly, so > in general, for long-term maintenance, I would recommend against using > vendored dependencies.) I tried building without bundling and there was two dependencies missing. I also could not figure out from the docs (https://docs.fedoraproject.org/en-US/packaging-guidelines/Rust/) how to build the package as it failed with `fedpkg local` and there are no mentions that using mock is required. I found https://discussion.fedoraproject.org/t/how-does-packaging-rust-binaries-for-fedora-work/22779 & https://discussion.fedoraproject.org/t/unable-to-build-spec-file-generated-by-rust2rpm/41090 that mention that only building with mock works with auto generated buildrequires. It would be good to update the docs to explain how to build unbundled Rust packages. This package was orphaned due to lack of time according to https://src.fedoraproject.org/rpms/rust-git-absorb. I don't have the time to maintain all the dependencies as separate packages in Fedora so I will only maintain this package if it's using bundled dependencies or if someone else maintains the dependencies in Fedora.
> it failed with `fedpkg local` Yeah, this is kind of by design, as you found - it just doesn't support modern RPM features properly. > there was two dependencies missing. If it's just two (turns out it would be three), then I don't think we'd consider that sufficient reason to build with vendored dependencies. Usually the low bar the Rust SIG applies for "too many missing dependencies" would be ~10+ or ~20+, depending on the complexity. It looks like what's missing are these three: - iobuffer 0.2.0 - slog-extlog 8.1.0 - slog-json 2.6.1 They are also *only* used in test code. If you build with `%bcond check 0` there are no missing dependencies. If you prefer running tests in this package, then I can submit the missing three dependencies for review. They look like mostly straightforward crates and should not be much work for the Rust SIG to maintain.
Updated specfile & SRPM fixing the build & man page generation: Spec URL: https://siosm.fedorapeople.org/srpms/rust-git-absorb.spec SRPM URL: https://siosm.fedorapeople.org/srpms/rust-git-absorb-0.9.0-4.fc45.src.rpm Branch: https://src.fedoraproject.org/fork/siosm/rpms/rust-git-absorb/tree/main-0.9.0
> > there was two dependencies missing. > > If it's just two (turns out it would be three), then I don't think we'd > consider that sufficient reason to build with vendored dependencies. Usually > the low bar the Rust SIG applies for "too many missing dependencies" would > be ~10+ or ~20+, depending on the complexity. > > It looks like what's missing are these three: > > - iobuffer 0.2.0 > - slog-extlog 8.1.0 > - slog-json 2.6.1 > > They are also *only* used in test code. If you build with `%bcond check 0` > there are no missing dependencies. > > If you prefer running tests in this package, then I can submit the missing > three dependencies for review. They look like mostly straightforward crates > and should not be much work for the Rust SIG to maintain. That's your (or the Rust SIG) decision. My preference is keeping tests and bundling but I will abide by the SIG/your decision as long as I don't have to maintain un-bundled dependencies.
Created attachment 2133368 [details] The .spec file difference from Copr build 10218157 to 10224713
Copr build: https://copr.fedorainfracloud.org/coprs/build/10224713 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2446933-rust-git-absorb/fedora-rawhide-x86_64/10224713-rust-git-absorb/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++/ - A package with this name already exists. Please check https://src.fedoraproject.org/rpms/rust-git-absorb Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_conflicting_package_names 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/10224758 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2446933-rust-git-absorb/fedora-rawhide-x86_64/10224758-rust-git-absorb/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++/ - A package with this name already exists. Please check https://src.fedoraproject.org/rpms/rust-git-absorb Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_conflicting_package_names 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.
I'll ask other Rust SIG members if they want to help getting the not-even-a-handful of missing dependencies for running tests packaged. If I get no takers, I'll do it myself ASAP.