Bug 478852 - Review Request: lpairs - Classical memory game with cards
Review Request: lpairs - Classical memory game with cards
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Alexey Torkhov
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-05 11:54 EST by Marcin Zajaczkowski
Modified: 2009-03-14 19:24 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-11 16:30:31 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
atorkhov: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Marcin Zajaczkowski 2009-01-05 11:54:11 EST
Spec URL: http://timeoff.wsisiz.edu.pl/rpms/lpairs/lpairs.spec
SRPM URL: http://timeoff.wsisiz.edu.pl/rpms/lpairs/lpairs-1.0.4-2.src.rpm
Description: 

Hi. Recently I have found that simple memory game which brought a reminiscence of an old Win 3.1 game Amnesia.

LPairs is a classical memory game. This means you have to find pairs of identical cards which will then be removed. Your time and tries needed will be counted but there is no highscore chart or limit to this.
Comment 1 Alexey Torkhov 2009-03-03 03:20:21 EST
- Avoid use of %makeinstall macros if "make install DESTDIR=%{buildroot}" working:
https://fedoraproject.org/wiki/Packaging/Guidelines#Why_the_.25makeinstall_macro_should_not_be_used

- Add desktop-file-utils to BuildRequires.

- Remove "--vendor fedora" from desktop-file-install call as per guidelines.

- Running update-desktop-database in %post and %postun is not needed as desktop entry doesn't has 'MimeType' key:
https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#desktop-database
Comment 2 Michael Schwendt 2009-03-03 06:16:12 EST
> Requires:	SDL >= 1.0

Ancient. Really should be removed in favour of the automatic libsdl soname dependency.


> %configure
[...]
> %makeinstall inst_dir="%{buildroot}%{_datadir}/%{name}"

Rather:

%configure inst_dir="%{_datadir}/%{name}"
[...]
make DESTDIR=%{buildroot} inst_dir="%{_datadir}/%{name}" install


> %defattr(0644,root,root,0755)

%defattr(-,root,root,-) won't work? Why?


* desktop-file-install warns about the Icon= entry
Comment 3 Marcin Zajaczkowski 2009-03-03 14:30:16 EST
Thanks guys for your suggestion.

I made new SPEC with following changes:
 - replaced %makeinstall
 - added desktop-file-utils to BuildRequires
 - removed "--vendor fedora"
 - removed update-desktop-database
 - removed SDL from Requires

http://timeoff.wsisiz.edu.pl/rpms/lpairs/lpairs.spec
http://timeoff.wsisiz.edu.pl/rpms/lpairs/lpairs-1.0.4-3.src.rpm

> %defattr(-,root,root,-) won't work? Why?

Probably works. I have never used them. I started using RPMs when original packages were made only by RedHat and there were many user pages with packages. I was safer in my opinion to force mentioned rights (especially for suided files).
Are there situations where (-,root,root,-) is better? Automatic builds uses (0644,root,root,0755) anyway.

> * desktop-file-install warns about the Icon= entry

Probably I have too old desktop-file-install (Fedora 8), but I didn't see any warning. Is it about .png extension?
If yes I would have to patch original file or create a new one. Is it worth to do that?
Comment 4 Alexey Torkhov 2009-03-03 14:49:12 EST
(In reply to comment #3)
> > %defattr(-,root,root,-) won't work? Why?
> 
> Probably works. I have never used them. I started using RPMs when original
> packages were made only by RedHat and there were many user pages with packages.
> I was safer in my opinion to force mentioned rights (especially for suided
> files).
> Are there situations where (-,root,root,-) is better? Automatic builds uses
> (0644,root,root,0755) anyway.

Well, by guidelines you should use (-,root,root,-) "unless you have a very good reason to deviate from that":
https://fedoraproject.org/wiki/Packaging/Guidelines#File_Permissions

> > * desktop-file-install warns about the Icon= entry
> 
> Probably I have too old desktop-file-install (Fedora 8), but I didn't see any
> warning. Is it about .png extension?
> If yes I would have to patch original file or create a new one. Is it worth to
> do that?

Yes, short name should be without extension.


Add "CC-BY-SA" to License tag as package includes some of tango icons.

License status of fonts and sounds must be clarified.
Comment 5 Michael Schwendt 2009-03-04 02:07:34 EST
%configure inst_dir="%{_datadir}/%{name}"

makes Patch0 obsolete, btw.
Comment 6 Marcin Zajaczkowski 2009-03-04 15:56:08 EST
Hmm, (In reply to comment #5)
> %configure inst_dir="%{_datadir}/%{name}"
> 
> makes Patch0 obsolete, btw.

Unfortunately it seems to be not so easy. A package without a patch is build, but there is problem, because an application tried to read grapthic file from "/usr/share/games/lpairs/gfx/".
There is an entry "inst_dir=$datadir/games/lpairs" in configure.in which is used by the application which I don't know how to override without patch.

Current SPEC:
http://timeoff.wsisiz.edu.pl/rpms/lpairs/lpairs.spec
http://timeoff.wsisiz.edu.pl/rpms/lpairs/lpairs-1.0.4-4.src.rpm


I also sent an email to the author with a question about fonts and sounds license.
Comment 7 Alexey Torkhov 2009-03-04 16:53:06 EST
> - added desktop-file-utils to BuildRequires
That was placed in Requires, not BuildRequires.

> - changed defattr to those recomended by packaging guidelines
%attr(0755,root,root) is unnecessary now too.

> If yes I would have to patch original file or create a new one. Is it worth to
> do that?
Not fixed?

Will do full review after fonts&sounds license clarified.
Comment 8 Marcin Zajaczkowski 2009-03-06 15:40:37 EST
> That was placed in Requires, not BuildRequires.

Ehh, probably it was late...

> %attr(0755,root,root) is unnecessary now too.

Ok, I will change it as well.

>> If yes I would have to patch original file or create a new one. Is it worth to
>> do that?
> Not fixed?

As I wrote earlier I'm sure if does it make sense to patch an old .desktop file/attach additional one to make an icon entry without .svg extension which doesn't make any effect, because in a package there is a svg file only anyway.


I've got an answer from the author:
<QUOTE>
Sounds stem from LBreakout2 which in turn took them from a CD with
non-copyrighted stuff I once bought: "Web Clip Empire 50.000",
NovaMedia Verlag, Germany. Some are also made by myself.

Fonts have been made by myself by printing X standard fonts and
modifying the bitmaps for shades and colors.

Graphics now com from Tango Desktop Project, thus are licensed under
"Creative Commons Attribution-Share Alike". Before 1.0.4 I used KDE
icons for the cards and pictures from http://www.grsites.com/textures
as backgrounds.
</QUOTE>
Comment 9 Alexey Torkhov 2009-03-06 16:18:11 EST
(In reply to comment #8)
> I've got an answer from the author:

Ok. Then, in my envision, correct License tag would be adding license for sounds: "GPLv2+ and CC-BY-SA and Freely redistributable without restriction".

> As I wrote earlier I'm sure if does it make sense to patch an old .desktop
> file/attach additional one to make an icon entry without .svg extension which
> doesn't make any effect, because in a package there is a svg file only anyway.

By guidelines it should be either short icon without extension or full path
https://fedoraproject.org/wiki/Packaging/Guidelines#Icon_tag_in_Desktop_Files
Comment 10 Marcin Zajaczkowski 2009-03-07 13:10:57 EST
In the newest version I clarified a license tag, removed attr, patched .desktop file and fixed desktop-file-utils.

http://timeoff.wsisiz.edu.pl/rpms/lpairs/lpairs.spec
http://timeoff.wsisiz.edu.pl/rpms/lpairs/lpairs-1.0.4-5.src.rpm
Comment 11 Alexey Torkhov 2009-03-07 13:34:52 EST
- rpmlint output:

lpairs.src:77: W: macro-in-%changelog pre
lpairs.src: W: mixed-use-of-spaces-and-tabs (spaces: line 13, tab: line 1)
lpairs.src: W: non-coherent-filename lpairs-1.0.4-5.src.rpm
lpairs-1.0.4-5.fc8.src.rpm
3 packages and 0 specfiles checked; 0 errors, 3 warnings.

Please clean up mixed spaces and tabs; and macros in changelog.

+ 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.
+ File, containing the text of the licenses 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 matches the upstream source, as
  provided in the spec URL.

5eb00da9f7fc15dc5ce840c312e76cfa  lpairs-1.0.4.tar.gz
5eb00da9f7fc15dc5ce840c312e76cfa  lpairs-1.0.4.tar.gz.orig

+ The package successfully compiles and builds into binary rpms on at least
  one primary architecture (x86_64).
+ All build dependencies are listed in BuildRequires.
+ The spec file handles locales properly.
+ Does not contain shared libraries.
+ The package does not designed to be relocatable.
+ A package owns all directories that it creates.
+ A package does not contain any duplicate files in the %files listing.
+ Permissions on files are set properly.
+ The package has a %clean section, which contains rm -rf %{buildroot}.
+ The package consistently uses macros.
+ The package contains code, or permissible content.
+ Does not contain large documentation files.
+ Includes only doc files in %doc.
+ No headers.
+ No static libraries.
+ The package does not contain pkgconfig(.pc) files.
+ The package does not contain library files with a suffix (e.g.
  libfoo.so.1.1).
+ No devel packages.
+ The package does not contain any .la libtool archives.
+ Includes %{name}.desktop file. Properly installed with desktop-file-install.
+ The package does not own files or directories already owned by other
  packages.
+ At the beginning of %install, the package runs rm -rf %{buildroot}.
+ All filenames in the package are valid UTF-8.

SHOULD:
Source package does not include CC-BY-SA license text and license for sounds as
a separate file from upstream, should query upstream to include it.


This package is APPROVED.
Comment 13 Marcin Zajaczkowski 2009-03-07 14:30:31 EST
New Package CVS Request
=======================
Package Name: lpairs
Short Description: Classical memory game with cards
Owners: szpak
Branches: F-9 F-10
InitialCC:
Comment 14 Kevin Fenzi 2009-03-09 11:54:43 EDT
cvs done.
Comment 15 Marcin Zajaczkowski 2009-03-11 16:30:31 EDT
Imported and built into devel, F-9 and F-10 branches. Package updates will be requested soon.

Thanks for your help!
Comment 16 Alexey Torkhov 2009-03-12 00:46:30 EDT
Don't forget to add comps.xml.
Comment 17 Marcin Zajaczkowski 2009-03-14 19:24:34 EDT
Thanks for a hint. I added it also there.

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