Bug 699843

SRPM URL: http://sourceforge.net/projects/dspaceinvadors/files/dsi-1.0.7-beta2/dsi-1.0.7-1.fc14.src.rpm

Since I have more review experience and the Fedora release has ticked over, I intend to post updated SRPM and SPEC links soon.

Damian, what's the status of this package? Would you still like to submit an updated version?

Damian, are you still interested in this package? If not, please close this ticket as NOTABUG.

Hi Martin

I'm just back from a break. Yes, I am still interested in this package. I'll be working on this during the next few days.

Thanks
Damian

OK, great. Thanks for the feedback.

I'll re-prioritise DSI and expect to have time available soon (I've also been working to recertify for RHCE on RHEL6, nearly there) - it is important to make small games available in Fedora and also useful as a marketing multiplier for Interlinux projects.

This submission will also help fix the maintainers knowledge in my mind.

Hi Martin

I have spent some time bringing the dsi.spec closer in-line with package review guidelines. I'm expecting mistakes need to be corrected:)

SPEC URL: 
http://dspaceinvadors.svn.sourceforge.net/viewvc/dspaceinvadors/dsi.spec?revision=208

SRPM URL:
http://sourceforge.net/projects/dspaceinvadors/files/dsi-1.0.7-beta2/dsi-1.0.7-2.fc15.src.rpm

[makerpm@fedora15 SPECS]$ rpmlint -i dsi.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
[makerpm@fedora15 SRPMS]$ rpmlint -i dsi-1.0.7-2.fc15.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Notes:

This is more complex than the previous package we worked on, a compiled C application with desktop elements. Please bear with me while mistakes are corrected. DSI broadens my scope as a fedora maintainer.

Here are some technical issues to clarify:
-I have set the platform to be i386, the lowest common denominator. Can this be the default for compatible platforms, i.e. i686?
-I have used macros throughout, apart from CFLAGS="$RPM_OPT_FLAGS" does this need to be changed?
-BuildRequires and Requires is present, with requirements, is this correct?
-Is a man page required?
-The resulting rpm installs and runs correctly on fedora15, as expected, but there is a warning about a world writeable file when rpmlint is run against the rpm: 

"dsi.i386: E: world-writable /var/lib/games/dsi/hiscore 0666L
A file or directory in the package is installed with world writable
permissions, which is most likely a security issue."

should I change this?

-There are most likely more issues to resolve, I appreciate your feedback, I am also reviewing documentation to cover new ground.

Damian

Hi Damian,

you spec file doesn't look bad, it just needs some streamlining.


> -I have set the platform to be i386, the lowest common denominator. Can this be
> the default for compatible platforms, i.e. i686?

Just remove the BuildArch field completely. It's set by rpmbuild depending on the selected target system. By default, koji creates both i686 and x86_64 packages, so you shouldn't hard-code the build arch here.


> -I have used macros throughout, apart from CFLAGS="$RPM_OPT_FLAGS" does this
> need to be changed?

Using the variable instead of the corresponding macro %{buildarch} is fine. However, the definition of CFLAGS is not required here. It's already part of the %configure macro and therefore recognized by make. You can verify it with rpm --eval %configure


> -BuildRequires and Requires is present, with requirements, is this correct?

The BuildRequires (BR) field is almost correct. The package fails to build in mock because of the missing BR desktop-file-utils.
I also recommend to use multiple BR lines with a single package name each rather than one line in order to improve legibility. 

You can drop all Requires as these dependencies are recognized automatically (due to the corresponding linked shared libraries).


> -Is a man page required?

No, it's optional but nice to have if one is available and if it contains some useful information.


> "dsi.i386: E: world-writable /var/lib/games/dsi/hiscore 0666L
> A file or directory in the package is installed with world writable
> permissions, which is most likely a security issue."
> 
> should I change this?

Yes, I think that's a good idea. I suggest to assign this file to group "games" like the data files of gnuchess, and set the permissions to 0664. 
All users who want to play dsi have to add themselves to the games group first.


Here are some further things I recognized so far:

- According to the source file headers, the license is GPLv2+. The "+" 
  indicates the addition "or (at your option) any later version". Thus, update
  the License field accordingly.

- I suggest to run "make install" prior to the additional cp/install commands.
  Also, remove all the appended variable definitions except DESTDIR. As far as
  I can see, none of them is required.

- Remove all the install lines as they are not necessary. You can install the
  icon and the hiscore file with
  install -D alien.png %{buildroot}%{_datadir}/pixmaps/alien.png
  install -D hiscore %{buildroot}%{_localstatedir}/lib/games/%{name}/hiscore

- The spec addresses two different .desktop files: that one provided by the
  tarball and another one created with "cat". Since dsi already ships a .desktop
  file, you don't need to create one. 
  * remove "cp -av DSI.desktop ..."
  * remove the heredoc creating dsi.desktop
  * rename DSI.desktop to dsi.desktop 
  * install it with desktop-file-install

- You should also remove the .png suffix from the Icon entry in the .desktop
  file. It's determined automatically and considered an error by 
  desktop-file-validate. Also, remove the deprecated Encoding entry.

- Drop "%define _unpackaged_files_terminate_build 0". rpmbuild should fail
  if some of the installed files are not packaged.

- Please be a bit more specific in %files and avoid plain asterisks. It 
  improves legibility and helps to prevent adding unwanted files.
  %{_bindir}/* => %{_bindir}/DSI

- Add "%dir %{_localstatedir}/lib/games/%{name}/" to the %file section for
  proper directory ownership.

- Add ChangeLog and TODO to the %doc line in %files.(In reply to comment #8)

Thanks Martin, digesting this...

Starting a new role tomorrow (support dev/ops Linux Engineer). Also about to install F16 this week. back on this as soon as re-organised - won't be long.

 -- Damian

Hi Martin

So far, I have made these changes - as suggested above:

- GPLv2+ used with +
- removed BuildArch field
- removed definition of CFLAGS
- dropped Requires
- multiple lines for BR
- added BR desktop-file-utils
- to avoid world writeable permissions on dat file hiscore;
   in %install used: install -m 644 -p hiscore
   in %files used: %attr(644,root,games)
- in %install make install run before additional cp/install commands
- variable definitions removed except DESTDIR
- removed install lines
- changed icon and hiscore to use install

After these changes (one or more may not be accurate) I have run into build an error, I am unsure how to resolve. I looks like the hiscore file is not located after the changes ...

SPEC URL: http://dspaceinvadors.svn.sourceforge.net/viewvc/dspaceinvadors/dsi.spec?revision=209

run against:

http://sourceforge.net/projects/dspaceinvadors/files/dsi-1.0.7-beta2/dsi-1.0.7.tar.gz

Error
-----

[makerpm@nodea SPECS]$ rpmbuild -ba dsi.spec 

 ...

 /usr/bin/install -c -m 644 dsirc '/home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/usr/share/dsi'
test -z "/home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/var/lib/games/dsi" || /bin/mkdir -p "/home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/var/lib/games/dsi"
 /usr/bin/install -c -m 644 hiscore '/home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/var/lib/games/dsi'
make  install-data-hook
make[3]: Entering directory `/home/makerpm/rpmbuild/BUILD/dsi-1.0.7'
chmod 666 /home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/var/lib/games/dsi/hiscore
chmod: cannot access `/home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/var/lib/games/dsi/hiscore': No such file or directory
make[3]: *** [install-data-hook] Error 1
make[3]: Leaving directory `/home/makerpm/rpmbuild/BUILD/dsi-1.0.7'
make[2]: *** [install-data-am] Error 2
make[2]: Leaving directory `/home/makerpm/rpmbuild/BUILD/dsi-1.0.7'
make[1]: *** [install-am] Error 2
make[1]: Leaving directory `/home/makerpm/rpmbuild/BUILD/dsi-1.0.7'
make: *** [install-recursive] Error 1
error: Bad exit status from /var/tmp/rpm-tmp.WYkgQN (%install)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.WYkgQN (%install)

 ...


 -- Damian

Hello Damian,

(In reply to comment #12)
> After these changes (one or more may not be accurate) I have run into build an
> error, I am unsure how to resolve. I looks like the hiscore file is not located
> after the changes ...
>  /usr/bin/install -c -m 644 hiscore
> '/home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/var/lib/games/dsi'

> make  install-data-hook
> chmod: cannot access
> `/home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/var/lib/games/dsi/hiscore':

The origin of the above error is the dsi Makefile. As you can see above, it installs the hiscore file in %{buildroot}%{buildroot}/var/lib/games/dsi. Thus, chmod can't find it in %{buildroot}/var/lib/games/dsi.
To fix this, you should drop $(DESTDIR) from the definition of "scoresdir", and add it to the chmod statement in the install-data-hook rule (in Makefile.am).
As the Makefile already installs the hiscore file, you don't need to install it in the spec once more. 

Some further notes:
- Drop the .png suffix from the icon field in the .desktop file. The proper
  extension is detected automatically.

- Also, drop the whole "Encoding" field, and the category "Application" from
  the .desktop file.

$ desktop-file-validate dsi.desktop 
dsi.desktop: error: (will be fatal in the future): value "alien.png" for key "Icon" in group "Desktop Entry" is an icon name with an extension, but there should be no extension as described in the Icon Theme Specification if the value is not an absolute path
dsi.desktop: warning: value "Application;Game;ArcadeGame;" for key "Categories" in group "Desktop Entry" contains a deprecated value "Application"
dsi.desktop: warning: key "Encoding" in group "Desktop Entry" is deprecated


- Install the desktop file like this:
  desktop-file-install --dir %{buildroot}/%{_datadir}/applications \
    --delete-original \
     DSI.desktop

- instead of %{_localstatedir}/lib you can also use %{_sharedstatedir}

- Avoid using plain "*" in the %files sections, especially if only few files
  are added:
  %{_bindir}/* => %{_bindir}/DSI

- add %dir %{_sharedstatedir}/games/%{name}/ to the %files section for
  proper directory ownership

- drop the line "%define _unpackaged_files_terminate_build 0"

Hi Martin

I have made the suggested changes, dropping the line "%define _unpackaged_files_terminate_build 0"

left this error message (as expected):

SPEC URL
http://dspaceinvadors.svn.sourceforge.net/viewvc/dspaceinvadors/dsi.spec?revision=213

(run against latest source make dist)

 ...

Checking for unpackaged file(s): /usr/lib/rpm/check-files /home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386
error: Installed (but unpackaged) file(s) found:
   /home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/usr/share/applications/DSI.desktop
   /home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/usr/share/pixmaps/alien.png

RPM build errors:
    Installed (but unpackaged) file(s) found:
   /home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/usr/share/applications/DSI.desktop
   /home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/usr/share/pixmaps/alien.png

 ...

I still can't see why there is this error. Are these two lines the responsible? Or something before?

...
%{_datadir}/applications/DSI.desktop
%{_datadir}/pixmaps/alien.png
...

also, are these two lines set correctly, i.e. in the right order?

..
%dir %{_localstatedir}/lib/games/%{name}/
%{_datadir}/%{name}
..

Changing the name of DSI.desktop to dsi.desktop proved to be quite complicated in the Makefiles, is this strictly necessary?

Surprisingly dsi.spec is much smaller and easier to read - very nice.

I'll add another %changelog entry when the changes are completed.

Best Regards
Damian

Damian, could you please add a link to an SRPM every time you provide a new revision? Otherwise, it's hard to reproduce the issues and to review the package.


>     Installed (but unpackaged) file(s) found:
>   
> /home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/usr/share/applications/DSI.desktop
>   
> /home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/usr/share/pixmaps/alien.png
> I still can't see why there is this error. Are these two lines the responsible?
> Or something before?
> 
> ...
> %{_datadir}/applications/DSI.desktop
> %{_datadir}/pixmaps/alien.png

If these lines are present in the %files section, you shouldn't get the above errors. I need to have a look into the latest revision of the package. 


> also, are these two lines set correctly, i.e. in the right order?
> 
> ..
> %dir %{_localstatedir}/lib/games/%{name}/
> %{_datadir}/%{name}
> ..

Yes, these are fine. The order of the lines doesn't matter. I'd just replace %{_localstatedir}/lib with %{_sharedstatedir} here too to be consistent. Personally, I also prefer to append a trailing slash to paths that specify a folder. This way it's easier to visually distinguish between files and folders:
%{_datadir}/%{name} => %{_datadir}/%{name}/



> Changing the name of DSI.desktop to dsi.desktop proved to be quite complicated
> in the Makefiles, is this strictly necessary?

According to the guidelines, the desktop file must be named %{name}.desktop. You don't need to rename it in the Makefile. You can do it in the spec.

Again, please provide an SRPM of your package. Then I can have a more detailed look into the recent tarball and spec.

SPEC URL: http://dspaceinvadors.svn.sourceforge.net/viewvc/dspaceinvadors/dsi.spec

SRPM URL: http://sourceforge.net/projects/dspaceinvadors/files/dsi-1.0.7-beta2/dsi-1.0.7-2.fc16.src.rpm

To create the SRPM I needed to (temporarily) keep this line "%define _unpackaged_files_terminate_build 0"

I have completed all required & recommended changes apart from DSI.desktop mismatch with package name (see below).

Without _unpackaged_files_terminate_build this error still occurs:

 ...
Checking for unpackaged file(s): /usr/lib/rpm/check-files
/home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386
error: Installed (but unpackaged) file(s) found:  
/home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/usr/share/applications/DSI.desktop  
/home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/usr/share/pixmaps/alien.png
RPM build errors:
    Installed (but unpackaged) file(s) found:  
/home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/usr/share/applications/DSI.desktop  
/home/makerpm/rpmbuild/BUILDROOT/dsi-1.0.7-2.fc16.i386/usr/share/pixmaps/alien.png
 ...

Also, I'm unsure where in the spec DSI.desktop can be changed to dsi.desktop so that it is found at all stages of the packaging process...

 -- Damian

Additionally re comment #16, 

I installed the resulting dsi-1.0.7-2.fc16.i686.rpm and the menu item was in Applications->Games - and the game ran properly! The above error message appears a little cryptic. Noted this documentation: https://fedoraproject.org/wiki/Packaging/Guidelines#desktop which the dsi.spec follows.

- On the revision above:

rpmlint SRPMS/dsi-1.0.7-2.fc16.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

rpmlint SPECS/dsi.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

rpmlint RPMS/i686/dsi-1.0.7-2.fc16.i686.rpm 
dsi.i686: W: no-manual-page-for-binary DSI
1 packages and 0 specfiles checked; 0 errors, 1 warnings.


- A man page is not a priority for this release.

- Bump the revision to 1.0.7-3 with the next release.

- I'll paste a list of MUST and SHOULD items (https://fedoraproject.org/wiki/Packaging:ReviewGuidelines)

 -- Damian

Created attachment 582424 [details]
fix packaging issues

Hello Damian,

you don't need to post a MUST/SHOULD checklist of your own package submissions. That's the job of the reviewer. :) It's nonetheless good practice to go through the items privately in order to reduce the number of review iterations.

I've attached a patch that should fix the issues with your latest package. Here is what it does:
- remove the $(DESTDIR) prefixes from desktopdir and pixmapsdir in Makefile.am
  (maybe you might want to fix this upstream)
- rename DSI.desktop to %{name}.desktop
- remove the "install" statements as the files are already installed by 
  "make install"
- replace desktop-file-install with desktop-file-validate

Hi Martin

Thank you for your patch. I applied changes upstream in Makefile.am - tested with make install and everything was fine. So I have committed to the dev trunk. The remainder of the patch was applied to dsi.spec giving a clean build. The version has been bumped one. 

SPEC URL: http://dspaceinvadors.svn.sourceforge.net/viewvc/dspaceinvadors/dsi.spec

SRPM URL: http://sourceforge.net/projects/dspaceinvadors/files/dsi-1.0.7-beta2/dsi-1.0.7-3.fc16.src.rpm 

rpmlint SPECS/dsi.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
rpmlint SRPMS/dsi-1.0.7-3.fc16.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

 -- Damian

Hi Martin

I would like to credit you for your work on this package which has become part of the main source trunk.

Have sent a separate email.

 -- Damian

Damian, sorry for the delay -- I'm pretty with my job busy at the moment. 
Here's the formal review of your package. It looks good now and is ready for check-in. 


$ rpmlint /var/lib/mock/fedora-16-x86_64/result/*.rpm
dsi.x86_64: W: no-manual-page-for-binary DSI
3 packages and 0 specfiles checked; 0 errors, 1 warnings.

---------------------------------
key:

[+] OK
[.] OK, not applicable
[X] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license.
[+] MUST: The License field in the package spec file must match the actual license.
    - GPLv2+ according to source file headers

[+] MUST: The file containing the text of the license(s) for the package must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source.
    $ md5sum dsi-1.0.7.tar.gz*
    720589cec25b0e37b76bfdae8ecf80d0  dsi-1.0.7.tar.gz
    720589cec25b0e37b76bfdae8ecf80d0  dsi-1.0.7.tar.gz.upstream

[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
[.] MUST: If the package does not successfully compile, build or work on an architecture, ...
[+] MUST: All build dependencies must be listed in BuildRequires.
[+] MUST: When compiling C, C++, or Fortran files, %{optflags} must be applied.
[.] MUST: The spec file MUST handle locales properly.
[.] MUST: If a package installs files below %{_datadir}/icons, the icon cache must be updated.
[.] MUST: Packages storing shared library files (not just symlinks) must call ldconfig in %post and %postun.
[+] MUST: Packages must NOT bundle copies of system libraries.
[.] MUST: If the package is designed to be relocatable, ...
[+] MUST: A package must own all directories that it creates. 
[+] MUST: A Fedora package must not list a file more than once in %files.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.
[.] MUST: Large documentation files must go in a -doc subpackage.
[+] MUST: Files in %doc must not affect the runtime of the application.
[.] MUST: Header files must be in a -devel package.
[.] MUST: Static libraries must be in a -static package.
[.] MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), ...
[.] MUST: devel packages must require the base package using a fully versioned dependency.
[+] MUST: Packages must NOT contain any .la libtool archives.
[+] MUST: Packages containing GUI applications must include a %{name}.desktop file. 
[+] MUST: .desktop files must be properly installed with desktop-file-install in the %install section. 
[+] MUST: Packages must not own files or directories already owned by other packages.
[+] MUST: All filenames in rpm packages must be valid UTF-8.

EPEL <= 5 only:
[+] MUST: The spec file must contain a valid BuildRoot field.
[+] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot}.
[+] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot}.
[.] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'

[.] SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
[+] SHOULD: Timestamps of files should be preserved.
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The package should compile and build into binary rpms on all supported architectures.
[+] SHOULD: The reviewer should test that the package functions as described.
[.] SHOULD: If scriptlets are used, those scriptlets must be sane.
[.] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
[.] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg.
[.] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.
[X] SHOULD: Your package should contain man pages for binaries/scripts.

----------------
Package APPROVED
----------------

New Package SCM Request
=======================
Package Name: dsi
Short Description: Invading aliens game using SDL
Owners: dbrasher
Branches: f15 f16 f17
InitialCC:

Git done (by process-git-requests).

dsi-1.0.7-3.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/dsi-1.0.7-3.fc15

dsi-1.0.7-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/dsi-1.0.7-3.fc16

dsi-1.0.7-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/dsi-1.0.7-3.fc17

dsi-1.0.7-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/dsi-1.0.7-3.fc16

dsi-1.0.7-3.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/dsi-1.0.7-3.fc15

dsi-1.0.7-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/dsi-1.0.7-3.fc17

dsi-1.0.7-3.fc17 has been pushed to the Fedora 17 testing repository.

dsi-1.0.7-3.fc17 has been pushed to the Fedora 17 stable repository.

dsi-1.0.7-3.fc16 has been pushed to the Fedora 16 stable repository.

dsi-1.0.7-3.fc15 has been pushed to the Fedora 15 stable repository.