https://bugzilla.redhat.com/show_bug.cgi?id=1565848 --- Comment #25 from dan.cermak@xxxxxxxxxxxxxxxxxxx --- Spec URL: https://pagure.io/Bear_rpm/raw/master/f/bear.spec SRPM URL: https://copr-be.cloud.fedoraproject.org/results/defolos/devel/fedora-29-x86_64/00828948-bear/bear-2.3.13-2.fc29.src.rpm (In reply to Till Hofmann from comment #24) > Some remarks: > > - The license should be GPLv3+, not GPLv3. Done > > - As Robert-André pointed out, you don't need to Require python. Done > > - The shebang replacement should keep the timestamp. Your version is also > not safe, as it would substitute "/usr/bin/env python" anywhere in the file, > and it would replace "/usr/bin/env python3" by "/usr/bin/python33". I > usually use the following snippet in %install instead: > for f in %{buildroot}/%{_bindir}/* ; do > sed -i.orig "s:^#\!/usr/bin/env\s\+python\s\?$:#!%{__python3}:" $f > touch -r $f.orig $f > rm $f.orig > done > > Note that if the shebang is "/usr/bin/env python3", then that is fine, > because the mangler will automatically change it to "/usr/bin/python3", see > [1]. I have tried that, but the mangler chooses python2 instead and complains that it will become an error soon. I have instead opted to use your snippet instead. Unfortunately it does not preserve timestamps, but that is due to the bear python script being created from a cmake template file. > > - In the future, please follow the pattern with URLs to the SPEC and SRPM on > the first two lines, as in the original post. The URLs should point to a raw > SPEC and a directly downloadable SRPM. This allows a reviewer to use > fedora-review. Sorry, I forgot to do that. > > - The file section can be simplified, e.g., instead of > %dir %{_libdir}/bear/ > %{_libdir}/bear/libear.so > you can simply write > %{_libdir}/bear > Similarly for the docs. Done > > - You list some doc files twice. If you list a file such as "%doc > README.md", then you don't need to list it again. The build system already > installs those files, so either you list them with %doc %{_docdir}/bear, or > you remove them in %install and install them with %doc README.md etc. You > also have two copies of COPYING. No need to have it in the doc dir, although > that's not a big issue either. Fixed that, the %files section became even shorter. > > - Why do you have conditionals on %{?fedora}? I usually build every package on CentOS/RHEL too and the tests were failing there, so I deactivated them on non-Fedora (and therefore also the buildrequires of python3-lit, which is only needed for the tests). However, my recent build on CentOS failed, as rpmbuild isn't as clever on RHEL as it is on Fedora and it won't install the documentation properly. > > [1] https://fedoraproject.org/wiki/Packaging:Guidelines#Shebang_lines -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx
