Spec URL: https://jcline.fedorapeople.org/new-packages/sigul-pesign-bridge.spec SRPM URL: https://jcline.fedorapeople.org/new-packages/sigul-pesign-bridge-0.2.1-1.fc42.src.rpm Description: Drop-in replacement for pesign's daemon that bridges pesign-client requests to a Sigul server. Fedora Account System Username: jcline
Copr build: https://copr.fedorainfracloud.org/coprs/build/8591001 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2343280-sigul-pesign-bridge/fedora-rawhide-x86_64/08591001-sigul-pesign-bridge/fedora-review/review.txt Found issues: - Systemd service file(s) in sigul-pesign-bridge 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.
Whoops, I completely forgot to add the systemd scriptlets. Fixed: Spec URL: https://jcline.fedorapeople.org/new-packages/sigul-pesign-bridge.spec SRPM URL: https://jcline.fedorapeople.org/new-packages/sigul-pesign-bridge-0.2.1-1.fc42.src.rpm
Created attachment 2074685 [details] The .spec file difference from Copr build 8591001 to 8591037
Copr build: https://copr.fedorainfracloud.org/coprs/build/8591037 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2343280-sigul-pesign-bridge/fedora-rawhide-x86_64/08591037-sigul-pesign-bridge/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.
Taking this review.
Just a head's up that I've released a new version upstream that has a dependency that needs packaging (https://bugzilla.redhat.com/show_bug.cgi?id=2352566) and drops the dependency on the Python sigul CLI. I'll update this request shortly with the newer version, once the dependency gets accepted.
Updated to v0.3.0 which drops the dependency on the Python sigul package Spec URL: https://jcline.fedorapeople.org/new-packages/sigul-pesign-bridge.spec SRPM URL: https://jcline.fedorapeople.org/new-packages/sigul-pesign-bridge-0.3.0-1.fc43.src.rpm
Created attachment 2080577 [details] The .spec file difference from Copr build 8591037 to 8777693
Copr build: https://copr.fedorainfracloud.org/coprs/build/8777693 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2343280-sigul-pesign-bridge/fedora-rawhide-x86_64/08777693-sigul-pesign-bridge/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.
Initial spec review notes: > BuildRequires: [...] Is there a reason we're not using dynamic buildrequires here?
(In reply to Neal Gompa from comment #10) > Initial spec review notes: > > > BuildRequires: [...] > > Is there a reason we're not using dynamic buildrequires here? I just used whatever rust2rpm spat out and adjusted it for the systemd scriptlets. It does indeed appear there's a macro to autogenerate deps, but I don't see any flag on rust2rpm to enable it. I don't see documentation for the macro to allow you to skip test dependencies so that's possibly why. It appears at least one of the test dependencies I have (tracing-test) isn't available on EPEL so I'd prefer to leave it as-is for now, or import this version into EPEL and use dynamic buildrequires for Fedora releases with the test dependencies available.
Could you comment about this?
Err, Fabio please ^
errr ... dynamic buildrequires should be on by default unless you run rust2rpm on OpenSUSE (or an unrecognized other distro). It looks like you got the spec file rendered from the "plain" template, which gives you ancient defaults and no modern features, so likely not at all what you want. You can force rust2rpm to target spec file generation at fedora with "rust2rpm --target fedora", this should give much better results.
> It appears at least one of the test dependencies I have (tracing-test) isn't available on EPEL so I'd prefer to leave it as-is for now, or import this version into EPEL and use dynamic buildrequires for Fedora releases with the test dependencies available. This also shouldn't be a blocker, importing one package into EPEL is something that takes literally 10 minutes.
(BTW, the way to have %cargo_generate_buildrequires not generate test-dependencies is to set %bcond check to 0.)
Okay, switched to generate build dependencies. Spec URL: https://jcline.fedorapeople.org/new-packages/sigul-pesign-bridge.spec SRPM URL: https://jcline.fedorapeople.org/new-packages/sigul-pesign-bridge-0.3.0-1.fc43.src.rpm
Created attachment 2081743 [details] The .spec file difference from Copr build 8777693 to 8813742
Copr build: https://copr.fedorainfracloud.org/coprs/build/8813742 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2343280-sigul-pesign-bridge/fedora-rawhide-x86_64/08813742-sigul-pesign-bridge/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.
Alright, one more change: switch to the crates.io package and drop the awkward cd <project>. This also means adjusting the package name. Spec URL: https://jcline.fedorapeople.org/new-packages/rust-sigul-pesign-bridge.spec SRPM URL: https://jcline.fedorapeople.org/new-packages/rust-sigul-pesign-bridge-0.3.1-1.fc43.src.rpm
Created attachment 2081760 [details] The .spec file difference from Copr build 8813742 to 8814586
Copr build: https://copr.fedorainfracloud.org/coprs/build/8814586 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2343280-sigul-pesign-bridge/fedora-rawhide-x86_64/08814586-rust-sigul-pesign-bridge/fedora-review/review.txt Found issues: - Systemd service file(s) in sigul-pesign-bridge 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.
Spec review: > %{_sysconfdir}/sigul-pesign-bridge/ This needs to be "%dir %{_sysconfdir}/sigul-pesign-bridge", since you enumerate the file as a %config file already. > %{_mandir}/man1/sigul-pesign-bridge.1.gz ".gz" should be replaced with "*".
Thanks. - replaced gz with glob - added %dir to the conf directory Spec URL: https://jcline.fedorapeople.org/new-packages/rust-sigul-pesign-bridge.spec SRPM URL: https://jcline.fedorapeople.org/new-packages/rust-sigul-pesign-bridge-0.3.1-1.fc43.src.rpm
Copr build: https://copr.fedorainfracloud.org/coprs/build/8818653 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2343280-rust-sigul-pesign-bridge/fedora-rawhide-x86_64/08818653-rust-sigul-pesign-bridge/fedora-review/review.txt Found issues: - Systemd service file(s) in sigul-pesign-bridge 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.
> %post > %systemd_post sigul-pesign-bridge.service > > %preun > %systemd_preun sigul-pesign-bridge.service > > %postun > %systemd_postun_with_restart sigul-pesign-bridge.service These need to be attached to the correct binary package. They don't do anything right now.
(In reply to Neal Gompa from comment #26) > > %post > > %systemd_post sigul-pesign-bridge.service > > > > %preun > > %systemd_preun sigul-pesign-bridge.service > > > > %postun > > %systemd_postun_with_restart sigul-pesign-bridge.service > > These need to be attached to the correct binary package. They don't do > anything right now. Any pointer on where exactly they're supposed to be? I assumed after the "%package -n sigul-pesign-bridge" bit was okay.
You just need to add "-n sigul-pesign-bridge" to %post/%preun/%postun
Ah, right. Thanks. Spec URL: https://jcline.fedorapeople.org/new-packages/rust-sigul-pesign-bridge.spec SRPM URL: https://jcline.fedorapeople.org/new-packages/rust-sigul-pesign-bridge-0.3.1-1.fc43.src.rpm
Created attachment 2082004 [details] The .spec file difference from Copr build 8818653 to 8820199
Copr build: https://copr.fedorainfracloud.org/coprs/build/8820199 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2343280-rust-sigul-pesign-bridge/fedora-rawhide-x86_64/08820199-rust-sigul-pesign-bridge/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.
Package was generated with rust2rpm, simplifying the review. ✅ package contains only permissible content ✅ package builds and installs without errors on rawhide ✅ test suite is run and all unit tests pass ✅ latest version packaged ✅ license matches upstream specification and is acceptable for Fedora ✅ license file is included with %license in %files ✅ package complies with Rust Packaging Guidelines Package APPROVED. === Recommended post-import tasks: - set up package on release-monitoring.org: - add @rust-sig with "commit" access as package co-maintainer (should happen automatically) - add @infra-sig with "commit" access as package co-maintainer - set bugzilla assignee overrides to @rust-sig - track package in koschei for all built branches (should happen automatically once rust-sig is co-maintainer)
The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-sigul-pesign-bridge