Spec URL: https://download.copr.fedorainfracloud.org/results/errornointernet/par2cmdline-turbo/fedora-rawhide-x86_64/06265296-par2cmdline-turbo/par2cmdline-turbo.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/errornointernet/par2cmdline-turbo/fedora-rawhide-x86_64/06265296-par2cmdline-turbo/par2cmdline-turbo-1.0.1-4.fc40.src.rpm Description: A simple fork of the par2cmdline project that adds huge performance improvements on x86 and ARM. Fedora Account System Username: errornointernet
Copr build: https://copr.fedorainfracloud.org/coprs/build/6265300 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2231216-par2cmdline-turbo/fedora-rawhide-x86_64/06265300-par2cmdline-turbo/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.
Bumped version to 1.1.0 New .spec URL: https://download.copr.fedorainfracloud.org/results/errornointernet/par2cmdline-turbo/fedora-rawhide-x86_64/06357985-par2cmdline-turbo/par2cmdline-turbo.spec New .srpm URL: https://download.copr.fedorainfracloud.org/results/errornointernet/par2cmdline-turbo/fedora-rawhide-x86_64/06357985-par2cmdline-turbo/par2cmdline-turbo-1.1.0-2.fc40.src.rpm
Created attachment 1986233 [details] The .spec file difference from Copr build 6265300 to 6357991
Copr build: https://copr.fedorainfracloud.org/coprs/build/6357991 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2231216-par2cmdline-turbo/fedora-rawhide-x86_64/06357991-par2cmdline-turbo/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.
This is an automatic check from review-stats script. This review request ticket hasn't been updated for some time. We're sorry it is taking so long. If you're still interested in packaging this software into Fedora repositories, please respond to this comment clearing the NEEDINFO flag. You may want to update the specfile and the src.rpm to the latest version available and to propose a review swap on Fedora devel mailing list to increase chances to have your package reviewed. If this is your first package and you need a sponsor, you may want to post some informal reviews. Read more at https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group. Without any reply, this request will shortly be considered abandoned and will be closed. Thank you for your patience.
Cleaned up a few things. New .spec URL: https://errornointernet.fedorapeople.org/review-requests/par2cmdline-turbo/par2cmdline-turbo.spec New .srpm URL: https://errornointernet.fedorapeople.org/review-requests/par2cmdline-turbo/par2cmdline-turbo-1.1.1-1.fc40.src.rpm Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=122928206
Bumped to v1.2.0 and fixed up provides line New .spec URL: https://errornointernet.fedorapeople.org/review-requests/par2cmdline-turbo/par2cmdline-turbo.spec New .srpm URL: https://errornointernet.fedorapeople.org/review-requests/par2cmdline-turbo/par2cmdline-turbo-1.2.0-1.fc41.src.rpm Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=126695009
Created attachment 2061974 [details] The .spec file difference from Copr build 6357991 to 8373802
Copr build: https://copr.fedorainfracloud.org/coprs/build/8373802 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2231216-par2cmdline-turbo/fedora-rawhide-x86_64/08373802-par2cmdline-turbo/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.
Update to 1.3.0. New .spec URL: https://errornointernet.fedorapeople.org/review-requests/par2cmdline-turbo/par2cmdline-turbo.spec New .srpm URL: https://errornointernet.fedorapeople.org/review-requests/par2cmdline-turbo/par2cmdline-turbo-1.3.0-1.fc42.src.rpm Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=132236872
Created attachment 2088221 [details] The .spec file difference from Copr build 8373802 to 8992718
Copr build: https://copr.fedorainfracloud.org/coprs/build/8992718 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2231216-par2cmdline-turbo/fedora-rawhide-x86_64/08992718-par2cmdline-turbo/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.
@errornointernet, thanks for creating this review request. I quickly looked at the SPEC file and I think it one issue will be the conflicting names of binaries and manual pages. This is expected since par2cmdline-turbo is effectively a fork of the par2cmdline. The Fedora Packaging Guidelines's section on Common Conflicting Files Cases and Solutions (https://docs.fedoraproject.org/en-US/packaging-guidelines/Conflicts/#_common_conflicting_files_cases_and_solutions) recommends to talk to upstream maintainers and ask them to rename the binaries (and manual pages) to avoid the conflicts. Naturally, we could teach users how to create symlinks or aliases so they can use par2cmdline-turbo as a drop-in replacement for par2cmdline.
How about just a `Conflicts` line? It seems like the original par2cmdline is sort of being maintained again with a new 1.0.0 release, and par2cmdline-turbo's author has stated in https://github.com/animetosho/par2cmdline-turbo/issues/41 that > I'd say the upstream project is more aimed at being a 'reference' implementation of PAR2, instead of targeting platform specific optimisations for performance like this fork is. So I expect that this fork will continue to exist, as the goals of this and par2cmdline are a little different.
(In reply to Ryan from comment #14) > How about just a `Conflicts` line? It seems like the original par2cmdline is > sort of being maintained again with a new 1.0.0 release, and > par2cmdline-turbo's author has stated in > https://github.com/animetosho/par2cmdline-turbo/issues/41 that > > > I'd say the upstream project is more aimed at being a 'reference' implementation of PAR2, instead of targeting platform specific optimisations for performance like this fork is. > So I expect that this fork will continue to exist, as the goals of this and > par2cmdline are a little different. I agree, both implementations have their place and I could imagine both being installed at the same time to do e.g. benchmark comparison between the two implementations. Hence, I'm not in favor of adding 'Conflicts' but rather renaming par2cmdline-turbo's binary to e.g. par2turbo (and helpers to par2turbo-create, par2turbo-repair and par2turbo-verify).
New .spec URL: https://errornointernet.fedorapeople.org/review-requests/par2cmdline-turbo/par2cmdline-turbo.spec New .srpm URL: https://errornointernet.fedorapeople.org/review-requests/par2cmdline-turbo/par2cmdline-turbo-1.3.0-1.fc43.src.rpm Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=139343902
Created attachment 2116295 [details] The .spec file difference from Copr build 8992718 to 9841224
Copr build: https://copr.fedorainfracloud.org/coprs/build/9841224 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2231216-par2cmdline-turbo/fedora-rawhide-x86_64/09841224-par2cmdline-turbo/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.
My comments to the SPEC file: > BuildRequires: automake I don't think it is a good practice to recreate the project's scripts by running ./automake.sh below? I would rather just BuildRequire make. > %autosetup -p1 Since there are no patches, it should be enough to just call "%autosetup". > %build > ./automake.sh As said above, I would remove this line and just keep the "%configure" and "%make_build". Can you add a comment line here above to explain why this is being done? E.g.: # Renaming binaries and man page to avoid conflicts with the par2cmdline package. > mv %{buildroot}%{_bindir}/par2 %{buildroot}%{_bindir}/par2turbo > rm -f %{buildroot}%{_bindir}/par2create \ > %{buildroot}%{_bindir}/par2repair \ > %{buildroot}%{_bindir}/par2verify \ > %{buildroot}%{_bindir}/par2 Removing %{buildroot}%{_bindir}/par2 can be skipped since it has been renamed to par2turbo in the mv command above. > ln -s par2turbo %{buildroot}%{_bindir}/par2turbo-create > ln -s par2turbo %{buildroot}%{_bindir}/par2turbo-repair > ln -s par2turbo %{buildroot}%{_bindir}/par2turbo-verify > mv %{buildroot}%{_mandir}/man1/par2.1 %{buildroot}%{_mandir}/man1/par2turbo.1 In the next step, it would make sense to file an issue with the upstream and ask them to rename things there. > %check > %make_build check-TESTS I think "make check-TEST" should be sufficient.
Hi, thanks for the review! > > BuildRequires: automake > > I don't think it is a good practice to recreate the project's scripts by running ./automake.sh below? > I would rather just BuildRequire make. > > > %autosetup -p1 > > Since there are no patches, it should be enough to just call "%autosetup". > > > %build > > ./automake.sh > > As said above, I would remove this line and just keep the "%configure" and "%make_build". There isn't a configure script in the tarballs for par2cmdline-turbo, so I put that in there to generate it. Is there a better way? > Can you add a comment line here above to explain why this is being done? > E.g.: # Renaming binaries and man page to avoid conflicts with the par2cmdline package. > Removing %{buildroot}%{_bindir}/par2 can be skipped since it has been renamed to par2turbo in the mv command above. Both done :) > In the next step, it would make sense to file an issue with the upstream and ask them to rename things there. Here: https://github.com/animetosho/par2cmdline-turbo/issues/49
> In the next step, it would make sense to file an issue with the upstream and ask them to rename things there. Upstream's response: > This project is meant to be a drop-in replacement for par2cmdline, so keeping the same binary is necessary to replace the original. > Whilst the goals are different, they aim for the same functionality, thus I can't really see why someone would likely want both installed on their system. > I think most packaging handles this by marking this as conflicting with par2cmdline. Is this an issue?
(In reply to Ryan from comment #20) > Hi, thanks for the review! > > > > BuildRequires: automake > > > > I don't think it is a good practice to recreate the project's scripts by running ./automake.sh below? > > I would rather just BuildRequire make. > > > > > %autosetup -p1 > > > > Since there are no patches, it should be enough to just call "%autosetup". > > > > > %build > > > ./automake.sh > > > > As said above, I would remove this line and just keep the "%configure" and "%make_build". > > There isn't a configure script in the tarballs for par2cmdline-turbo, so I > put that in there to generate it. > Is there a better way? Oh, I see now. I didn't realize that par2cmdline-turbo's upstream diverged here and doesn't provide release tarballs that would pre-generate the configure script and include it in the tarball. Then let's leave it as is and please file an issue to upstream and ask them to consider creating release tarballs which include the configure script to ease the installation experience and improve consistency across various systems. > > > Can you add a comment line here above to explain why this is being done? > > E.g.: # Renaming binaries and man page to avoid conflicts with the par2cmdline package. > > > Removing %{buildroot}%{_bindir}/par2 can be skipped since it has been renamed to par2turbo in the mv command above. > > Both done :) > > > In the next step, it would make sense to file an issue with the upstream and ask them to rename things there. > > Here: https://github.com/animetosho/par2cmdline-turbo/issues/49 Thanks! I've followed up and commented there myself. Could you update the package to version 1.4.0 that has been released in the mean time? I think we should be in good position to add it to Fedora then.