https://bugzilla.redhat.com/show_bug.cgi?id=1464778 --- Comment #5 from Andrea Musuruane <musuruan@xxxxxxxxx> --- (In reply to Björn "besser82" Esser from comment #2) > Some other hints on the spec-file: > > > URL: http://www.oletus.fi/static/whichwayisup/ > > Source0: http://www.oletus.fi/static/whichwayisup/%{name}_b%{pkgversion}.zip > > URL should NOT end with a trailing slash '/') Actually the reverse is true. URL should end with a trailing slash, otherwise you'll get HTTP status 301. > and you can shorten and > simplify those really good using: > > URL: http://www.oletus.fi/static/${name} > Source0: %{url}/%{name}_b%{pkgversion}.zip I think this is less legible. > > %setup -qn %{name} > > %patch0 -p1 > > Using the `autosetup`-macro is preferred (and support since el5) for new > packages. It supports automatic patching and all: > > %autosetup -p 1 Right now there is just one patch. It was not the case when I started to write the spec file. Using autosetup all the patches need to have the same strip level. Therefore, at the time, I couldn't use this macro. > > > install -d %{buildroot}%{_bindir} > > install -m 755 -p %{SOURCE1} %{buildroot}%{_bindir}/%{name} > > This applies to most of the code using `install`. You can do all of this in > one line and mode SHOULD be set in four-digit octal (e.g. 0755 / 0644): > > install -Dpm 0755 %{SOURCE1} %{buildroot}%{_bindir}/%{name} I guess this is just a matter of taste. Moreover, 3 digits are fine to specify the file/directory permissions. From the install man page: -m, --mode=MODE set permission mode (as in chmod), instead of rwxr-xr-x And from the chmod man page: A numeric mode is from one to four octal digits (0-7), derived by adding up the bits with values 4, 2, and 1. Omitted digits are assumed to be leading zeros. Moreover the guidelines just state that "Permissions on files MUST be set properly [...]": https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions > > %{_mandir}/man6/* > > Should be: %{_mandir}/man6/%{name}.6* Why? There is only one file in that directory and it found correctly: $ rpm -ql whichwayisup | grep man /usr/share/man/man6/whichwayisup.6.gz Moreover, the guidelines just state that "When installing man pages, note that they should be installed uncompressed as the build system will compress them as needed. The compression method may change, so it is important to reference the pages in the %files section with a pattern that takes this into account" https://fedoraproject.org/wiki/Packaging:Guidelines#Manpages > I'm not too happy with the wrapper script… You shouldn't install that from > a static file as is… Instead you should auto-generate that from inside the > spec-file (don't forget to have BR: python2-devel): > > %{__cat} << EOF > %{buildroot}%{_bindir}/%{name} > #!/bin/sh > set -e > > pushd %{_datadir}/%{name} > %{__python2} -s run_game.py "$@" > popd > EOF I think that having the wrapper file (and desktop file, and appdata file, etc) as a different file improves readability a lot. Moreover, version control is more efficient using different files. > At last I'd recommend using rpm-defined macros instead of plain command > invocation (my personal preference, left open to you): > > * %{__sed} instead of `sed` > * %{__install} instead of `install` > * %{__cp} instead of `cp` > * %{__mkdir_p} instead of `mkdir -p` > * %{__rm} instead of `rm` > * %{__ln_s} instead of `ln -s` This is actually deprecated. Macro forms of system executables SHOULD NOT be used except when there is a need to allow the location of those executables to be configurable. For example, rm should be used in preference to %{__rm}, but %{__python} is acceptable. https://fedoraproject.org/wiki/Packaging:Guidelines#Macros > *** > > > Spec-file and srpm are no direct downloads: > > $ fedora-review -m fedora-rawhide-x86_64 -b 1464778 ; > INFO: Processing bugzilla bug: 1464778 > INFO: Getting .spec and .srpm Urls from : 1464778 > INFO: --> SRPM url: https://svgames.pl/fedora/whichwayisup-0.7.9-1.src.rpm > INFO: --> Spec url: > https://www.dropbox.com/sh/46vz8vz5rmxx0ac/AADLlEEIDcFazfpKuVQJcs53a/reviews/ > whichwayisup.spec > INFO: Using review directory: > /home/besser82/vm_shared/fedora/review/1464778-whichwayisup > INFO: Downloading .spec and .srpm files > error: line 1: Unknown tag: <!DOCTYPE html><html lang="en" > xmlns:fb="http://ogp.me/ns/fb#"; xml:lang="en" class="maestro" > xmlns="http://www.w3.org/1999/xhtml";> > ERROR: "Can't parse specfile: can't parse specfile\n" (logs in > /home/besser82/.cache/fedora-review.log) This is due to dropbox. Unluckily right now I have no other way to share spec and SRPM files. -- 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
