https://bugzilla.redhat.com/show_bug.cgi?id=1119197
Ben Rosser <rosser....@gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Assignee|nob...@fedoraproject.org |rosser....@gmail.com
Flags| |fedora-review?
--- Comment #18 from Ben Rosser <rosser....@gmail.com> ---
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@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
