Bug 985065 - Review Request: peg-solitaire - Board game played with pegs
Review Request: peg-solitaire - Board game played with pegs
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Peter Lemenkov
Fedora Extras Quality Assurance
:
Depends On: 989437
Blocks: qt-reviews
  Show dependency treegraph
 
Reported: 2013-07-16 13:18 EDT by Mario Blättermann
Modified: 2016-08-14 12:24 EDT (History)
5 users (show)

See Also:
Fixed In Version: peg-solitaire-2.0-4.fc20
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-11-08 22:33:44 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lemenkov: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Mario Blättermann 2013-07-16 13:18:06 EDT
Spec URL: http://mariobl.fedorapeople.org/Review/SPECS/peg-solitaire.spec
SRPM URL: http://mariobl.fedorapeople.org/Review/SRPMS/peg-solitaire-2.0-1.fc19.src.rpm

Description:
Peg solitaire is a board game for one player involving movement of pegs on
a board with holes. The program includes over 330 solitaire problems.
The program looks for new solutions and it is possible to raise new problems.

Fedora Account System Username: mariobl
Comment 1 Christopher Meng 2013-07-16 21:38:38 EDT
Hi,

1. I think your desktop file is incorrect.

[Desktop Entry]
Exec='/usr/games/peg-solitaire'

The path is wrong.

Icon=/usr/share/pixmaps/peg-solitaire.xpm

This kind of way to define a icon is not recommended now, is it possible to change to "Icon=peg-solitaire"?(I'm sorry I haven't tried it)

MimeType=

Well this is just a game and no new MIME types are being introduced, so you don't need to add %post{un} scriptlet to update the database.

2. In your spec I can find find_lang script, but you've commented out them, and there are language files in the tarball in fact. Please fix.

3. I can see "${RPM_BUILD_ROOT}" in your spec.

However as we can see from the guideline

http://fedoraproject.org/wiki/Packaging:Guidelines#UsingBuildRootOptFlags

You can use $RPM_BUILD_ROOT or %{buildroot} to define the buildroot, but now you use a special style "${RPM_BUILD_ROOT}"...

So, if you want to use macro style, change it to %{buildroot}; Otherwise please change it to variable style. Mix using 2 style is WRONG, and you mix them in one new style is WRONG again.
Comment 2 Mario Blättermann 2013-07-17 12:47:14 EDT
(In reply to Christopher Meng from comment #1)
> Hi,
> 
> 1. I think your desktop file is incorrect.
> 
> [Desktop Entry]
> Exec='/usr/games/peg-solitaire'
> 
> The path is wrong.
> 
> Icon=/usr/share/pixmaps/peg-solitaire.xpm
> 
> This kind of way to define a icon is not recommended now, is it possible to
> change to "Icon=peg-solitaire"?(I'm sorry I haven't tried it)
> 
> MimeType=
> 
> Well this is just a game and no new MIME types are being introduced, so you
> don't need to add %post{un} scriptlet to update the database.
>
First you should have a look at the patch. It makes sure that the desktop file becomes valid and the executable binary goes into /usr/bin instead of /usr/games. You refer to that file in the tarball. This is not usable, indeed.

Moreover, the scriptlet doesn't contain anything about mime types. It's only for updating the desktop database.

> 2. In your spec I can find find_lang script, but you've commented out them,
> and there are language files in the tarball in fact. Please fix.
> 
Please read the comment. The find_lang macro is useful and applicable when gettext (or even the qt translation chain) places the language files in /usr/share/locale/language_code/LC_MESSAGES/*. This is not the case, peg-solitaire behaves completely different from other packages. I was wondering if it is possible to get find_lang working here, or if it even makes sense to use it when all the language files are in the same folder anyway.

Well, if I'm forced to use it, any ideas would be welcome.

> 3. I can see "${RPM_BUILD_ROOT}" in your spec.
> 
> However as we can see from the guideline
> 
> http://fedoraproject.org/wiki/Packaging:Guidelines#UsingBuildRootOptFlags
> 
> You can use $RPM_BUILD_ROOT or %{buildroot} to define the buildroot, but now
> you use a special style "${RPM_BUILD_ROOT}"...
> 
> So, if you want to use macro style, change it to %{buildroot}; Otherwise
> please change it to variable style. Mix using 2 style is WRONG, and you mix
> them in one new style is WRONG again.

Fixed.

Spec URL: http://mariobl.fedorapeople.org/Review/SPECS/peg-solitaire.spec
SRPM URL: http://mariobl.fedorapeople.org/Review/SRPMS/peg-solitaire-2.0-2.fc19.src.rpm
Comment 3 Kevin Kofler 2013-07-17 18:56:25 EDT
For what it's worth, "${RPM_BUILD_ROOT}" is valid shell syntax, just unusual in this context (because $RPM_BUILD_ROOT is normally followed by a /, making the braces useless, and quoting it is also not supposed to be necessary).
Comment 4 Kevin Kofler 2013-07-17 19:22:26 EDT
As for the other 2 points, unfortunately, there are some differences between what is written in the wiki and what is actually true in practice (and all experienced packagers know):

> Moreover, the scriptlet doesn't contain anything about mime types. It's only
> for updating the desktop database.

Actually, update-desktop-database is only needed if the .desktop file claims any MIME types, otherwise it does nothing (and just wastes the user's time).

> Please read the comment. The find_lang macro is useful and applicable when
> gettext (or even the qt translation chain) places the language files in
> /usr/share/locale/language_code/LC_MESSAGES/*.

Actually, it looks for the files with a "find" in the whole RPM_BUILD_ROOT, so it doesn't matter where they are. Please try it (but don't forget the --with-qt --without-mo switches if this is using Qt rather than gettext translations), it should work.
Comment 5 Mario Blättermann 2013-07-18 12:54:24 EDT
(In reply to Kevin Kofler from comment #4)
> As for the other 2 points, unfortunately, there are some differences between
> what is written in the wiki and what is actually true in practice (and all
> experienced packagers know):
> 
> > Moreover, the scriptlet doesn't contain anything about mime types. It's only
> > for updating the desktop database.
> 
> Actually, update-desktop-database is only needed if the .desktop file claims
> any MIME types, otherwise it does nothing (and just wastes the user's time).
>
OK, then it has to be changed in the wiki. There must not be a difference between the "law" and the "practice". And in no case we may assume that an "experienced packager" knows about that. Then we could also assume a lot of more things, and we could shrink the guidelines to a minimum.

> > Please read the comment. The find_lang macro is useful and applicable when
> > gettext (or even the qt translation chain) places the language files in
> > /usr/share/locale/language_code/LC_MESSAGES/*.
> 
> Actually, it looks for the files with a "find" in the whole RPM_BUILD_ROOT,
> so it doesn't matter where they are. Please try it (but don't forget the
> --with-qt --without-mo switches if this is using Qt rather than gettext
> translations), it should work.

OK, I didn't know about the --without-mo switch. It's the first time I work on non-gettext stuff. I've added the macro with the following line:

%find_lang -f solitari.lang --with-qt --without-mo

But I get this error:

+ /usr/lib/rpm/find-lang.sh /home/mariobl/rpmbuild/BUILDROOT/peg-solitaire-2.0-3.fc19.x86_64 -f solitari.lang --with-qt --without-mo
No translations found for -f in /home/mariobl/rpmbuild/BUILDROOT/peg-solitaire-2.0-3.fc19.x86_64

The problem is, we don't have files named solitari.qm in separate folders for each supported language. All the qm files are in a single folder with the following naming scheme:

$ rpm -ql peg-solitaire
...
/usr/share/games/peg-solitaire/locales
/usr/share/games/peg-solitaire/locales/solitari.qm
/usr/share/games/peg-solitaire/locales/solitari_ca_ES.qm
/usr/share/games/peg-solitaire/locales/solitari_de_DE.qm
/usr/share/games/peg-solitaire/locales/solitari_en_EN.qm
/usr/share/games/peg-solitaire/locales/solitari_en_US.qm
/usr/share/games/peg-solitaire/locales/solitari_es_ES.qm
/usr/share/games/peg-solitaire/locales/solitari_eu_ES.qm
/usr/share/games/peg-solitaire/locales/solitari_fr_FR.qm
/usr/share/games/peg-solitaire/locales/solitari_gl_ES.qm
/usr/share/games/peg-solitaire/locales/solitari_it_IT.qm
/usr/share/games/peg-solitaire/locales/solitari_pl_PL.qm
/usr/share/games/peg-solitaire/locales/solitari_pt_BR.qm
/usr/share/games/peg-solitaire/locales/solitari_pt_PT.qm

As another attempt I tried to use a wildcard (solitari*.lang), but it failed again.

The guideline says: "Keep in mind that usage of %find_lang in packages containing locales is a MUST." But is it really worth the effort to patch the package to get this script working? Well, we use find_lang to avoid mistakes such as wrong folder ownerships and writing the translation files in a long list [1]. But peg-solitaire behaves different from the usual way, we should keep this in mind. And if I'm really forced to use it, my knowledge is probably too poor to patch the sources in a way that it works.

BTW, some of the translations are made by an automatized tool like Google Translator and are of bad quality, at least the German one. This is another reason to avoid the effort.

[1] http://fedoraproject.org/wiki/Packaging:Guidelines#Why_do_we_need_to_use_.25find_lang.3F
Comment 6 Mario Blättermann 2013-07-18 13:05:51 EDT
(In reply to Mario Blättermann from comment #5)
> OK, then it has to be changed in the wiki. There must not be a difference
> between the "law" and the "practice". And in no case we may assume that an
> "experienced packager" knows about that. Then we could also assume a lot of
> more things, and we could shrink the guidelines to a minimum.

Sorry for the blurb... Just found the correct usage in the scriptlets collection [1]:

"Use this when a desktop entry has a 'MimeType key."

But it is somewhat strage, though. Such a hint should be in the packaging guidelines, not in the scriptlets collection only.

[1] http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-database
Comment 7 Kevin Kofler 2013-07-18 19:09:27 EDT
> %find_lang -f solitari.lang --with-qt --without-mo

That's not valid %find_lang syntax. Try:
%find_lang solitari --with-qt --without-mo

In the %files list, you'll want to list:
%dir %{_datadir}/games/peg-solitaire/locales/
%{_datadir}/games/peg-solitaire/locales/solitari.qm
directly and add -f solitary.lang for the rest.
Comment 8 Christopher Meng 2013-07-18 20:06:34 EDT
(In reply to Mario Blättermann from comment #6)
> (In reply to Mario Blättermann from comment #5)
> > OK, then it has to be changed in the wiki. There must not be a difference
> > between the "law" and the "practice". And in no case we may assume that an
> > "experienced packager" knows about that. Then we could also assume a lot of
> > more things, and we could shrink the guidelines to a minimum.
> 
> Sorry for the blurb... Just found the correct usage in the scriptlets
> collection [1]:
> 
> "Use this when a desktop entry has a 'MimeType key."
> 
> But it is somewhat strage, though. Such a hint should be in the packaging
> guidelines, not in the scriptlets collection only.
> 
> [1]
> http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-database

Yep, you can raise a ticket to FPC later.
Comment 9 Mario Blättermann 2013-07-20 16:49:25 EDT
(In reply to Kevin Kofler from comment #7)
> > %find_lang -f solitari.lang --with-qt --without-mo
> 
> That's not valid %find_lang syntax. Try:
> %find_lang solitari --with-qt --without-mo
> 
> In the %files list, you'll want to list:
> %dir %{_datadir}/games/peg-solitaire/locales/
> %{_datadir}/games/peg-solitaire/locales/solitari.qm
> directly and add -f solitary.lang for the rest.

Still not working. Error message was:

+ /usr/lib/rpm/find-lang.sh /home/mariobl/rpmbuild/BUILDROOT/peg-solitaire-2.0-3.fc19.x86_64 solitari --with-qt --without-mo
No translations found for solitari in /home/mariobl/rpmbuild/BUILDROOT/peg-solitaire-2.0-3.fc19.x86_64

Here's my current spec file:
http://mariobl.fedorapeople.org/Review/SPECS/peg-solitaire.spec
Comment 10 Michael Schwendt 2013-07-22 04:43:19 EDT
Well, you can try to debug /usr/lib/rpm/find-lang.sh to find out whether it can handle those paths.

Also note there are bugs in it, such as: bug 729336 (find-lang.sh doesn't find all locales when --with-qt and --all-name are combined)
Comment 11 Mario Blättermann 2013-07-29 05:25:26 EDT
I've filed a bug against rpm.
Comment 12 Mario Blättermann 2013-10-09 15:01:32 EDT
(In reply to Mario Blättermann from comment #11)
> I've filed a bug against rpm.

The find_lang macro was actually working, but there was a problem in my spec file which has been solved. Here are the new files:

Spec URL: http://mariobl.fedorapeople.org/Review/SPECS/peg-solitaire.spec
SRPM URL: http://mariobl.fedorapeople.org/Review/SRPMS/peg-solitaire-2.0-4.fc19.src.rpm
Comment 13 Peter Lemenkov 2013-10-29 14:52:22 EDT
I'll review this.
Comment 14 Peter Lemenkov 2013-10-29 15:04:26 EDT
Koji scratchbuild for F-19 (just because it faster tenfold than F20/F21 with arm enabled)

http://koji.fedoraproject.org/koji/taskinfo?taskID=6113561

REVIEW:

Legend: + = PASSED, - = FAILED, 0 = Not Applicable

+ rpmlint is not silent, but the only message may be ignored now:

Auriga ~/Desktop: rpmlint peg-solitaire-*
peg-solitaire.src:26: W: rpm-buildroot-usage %build qmake-qt4 PREFIX=%{buildroot}/%{_prefix} %{name}.pro

^^^ This should actually go into %install stage but I don't insist on fixing this right now.

3 packages and 0 specfiles checked; 0 errors, 1 warnings.
Auriga ~/Desktop: 

+ The package is named according to the  Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
+ The package meets the Packaging Guidelines.
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.
+ The License field in the package spec file matches the actual license (GPLv3 or later).
+ The file, containing the text of the license(s) for the package, is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package, match the upstream source, as provided in the spec URL.

sulaco ~/rpmbuild/SOURCES: sha256sum peg-solitaire-2.0.tar.gz*
71ac0a149a10c034051a7ac464fdff64205b45d770b29f76e9d251e6993cead3  peg-solitaire-2.0.tar.gz
71ac0a149a10c034051a7ac464fdff64205b45d770b29f76e9d251e6993cead3  peg-solitaire-2.0.tar.gz.1
sulaco ~/rpmbuild/SOURCES: 

+ The package successfully compiles and builds into binary rpms on at least one primary architecture. See Koji link above.
+ All build dependencies are listed in BuildRequires.
+ The spec file handles locales properly (by using the %find_lang macro).
0 No shared library files in some of the dynamic linker's default paths.

+ The package does NOT bundle copies of system libraries.
0 The package is not designed to be relocatable.
+ The package owns all directories that it creates.
+ The package does not list a file more than once in the spec file's %files listings.
+ Permissions on files are set properly.
+ The package consistently uses macros.
+ The package contains code, or permissible content.
0 No extremely large documentation files.
+ Anything, the package includes as %doc, does not affect the runtime of the application.
0 No C/C++ header files.
0 No static libraries.
0 No pkgconfig(.pc) files.
0 The package doesn't contain library files without a suffix (e.g. libfoo.so) in some of the dynamic linker's default paths.
0 No devel sub-package.
+ The package does NOT contain any .la libtool archives.
+ The package includes a %{name}.desktop file, and this file is validated with desktop-file-validate in the %check section.
+ The package does not own files or directories already owned by other packages.
+ All filenames in rpm packages are valid UTF-8.


I don't see any issues, so this package is

APPROVED.
Comment 15 Mario Blättermann 2013-10-29 15:27:09 EDT
Many thanks for the review!

New Package SCM Request
=======================
Package Name: peg-solitaire
Short Description: Board game played with pegs
Owners: mariobl
Branches: f19 f20
Comment 16 Gwyn Ciesla 2013-10-30 08:00:28 EDT
Git done (by process-git-requests).
Comment 17 Fedora Update System 2013-10-31 05:02:16 EDT
peg-solitaire-2.0-4.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/peg-solitaire-2.0-4.fc20
Comment 18 Fedora Update System 2013-10-31 05:02:29 EDT
peg-solitaire-2.0-4.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/peg-solitaire-2.0-4.fc19
Comment 19 Fedora Update System 2013-10-31 13:39:32 EDT
peg-solitaire-2.0-4.fc20 has been pushed to the Fedora 20 testing repository.
Comment 20 Fedora Update System 2013-11-08 22:33:44 EST
peg-solitaire-2.0-4.fc19 has been pushed to the Fedora 19 stable repository.
Comment 21 Fedora Update System 2013-11-10 02:25:01 EST
peg-solitaire-2.0-4.fc20 has been pushed to the Fedora 20 stable repository.

Note You need to log in before you can comment on or make changes to this bug.