[Bug 1119197] Review Request: gnushogi - Shogi (Japanese Chess) AI engine

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite Conditions]     [KDE Users]

  Powered by Linux