https://bugzilla.redhat.com/show_bug.cgi?id=1119197 Ben Rosser <rosser.bjr@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@xxxxxxxxxxxxxxxxx |rosser.bjr@xxxxxxxxx Flags| |fedora-review? --- Comment #18 from Ben Rosser <rosser.bjr@xxxxxxxxx> --- Great! I have a few comments just based off of reading the spec. > Version: 1.5pre > Release: 1.git%{shortcommit}%{?dist} Fedora's Versioning guidelines say that to indicate a pre-release version, you should use the expected version number as the Version tag and put a leading "0." in the release tag. That is, something like this: Version: 1.5 Release: 0.1.git%{shortcommit}%{?dist} Then if and when upstream releases 1.5, you can remove the trailing 0 and rpm will see your package as an update. https://fedoraproject.org/wiki/Packaging:Versioning#Prerelease_versions > # The source of this package was pulled from upstreams's vcs. While this is okay to do when necessary, it's always better to use an actual SourceURL if possible. After poking around the Savannah cgit interface I believe you can use the following SourceURL: Source0: https://git.savannah.gnu.org/cgit/gnushogi.git/snapshot/gnushogi-%{commit}.tar.gz In which case, you'll need to modify the "setup" macro appropriately: %setup -qn %{name}-%{commit} And then you need to add the invocation of autogen.sh to the spec as well, along with the necessary BuildRequires on autoconf/automake/texinfo as well. > make %{?_smp_mflags} While this works fine, it is recommended to replace this with the relatively new "%make_build" macro. > %{__rm} -f %{buildroot}%{_infodir}/dir This is frowned upon. You should just use "rm": "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 > Requires: ncurses-libs This explicit dependency is probably unnecessary. RPM can generally figure out C/C++ library dependencies. You can check to make sure by running "rpm -qpR" over a spec to see the dependencies, like so: [bjr@rannoch Downloads]$ rpm -qpR /var/lib/mock/fedora-rawhide-x86_64/result/gnushogi-1.5pre-1.git5bb0b5b.fc28.x86_64.rpm /bin/sh /bin/sh libc.so.6()(64bit) libc.so.6(GLIBC_2.2.5)(64bit) libc.so.6(GLIBC_2.3.4)(64bit) libc.so.6(GLIBC_2.4)(64bit) libc.so.6(GLIBC_2.7)(64bit) libm.so.6()(64bit) libm.so.6(GLIBC_2.2.5)(64bit) libncurses.so.6()(64bit) ... So, rpm was able to find the libncurses dependency on it's own. In addition, according to https://fedoraproject.org/wiki/Packaging:Scriptlets#Texinfo you need to add the Requires for "info" for the post and preun triggers. > Requires(post): info > Requires(preun): info You might want to consider adding a weak dependency on xboard. You can read more about them here: https://fedoraproject.org/wiki/Packaging:WeakDependencies Otherwise the spec looks fine to me. I'll do a more detailed run through over the next few days and let you know if I find anything else. -- 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
