Bug 488124

My initial notes:

* The package fails to build (also the above koji links didn't give me anything). 
   File not found by glob: /builddir/build/BUILDROOT/gnubik-2.3-1.fc10.x86_64/usr/share/pixmap/gnubik.*
   File not found by glob: /builddir/build/BUILDROOT/gnubik-2.3-1.fc10.x86_64/usr/share/man/man*/gnubik*

This line looks problematic:
   install -Dp -m 644 doc/%{name}.6 $RPM_BUILD_ROOT%{_mandir}/man6/%{name}.6

Try to create the directory before installing the file.

* As far as I know, %{_datadir}/pixmaps is being deprecated and the new applications should install their pixmaps under %{_datadir}/icons/hicolor/<dim>x<dim>/apps/
where <dim> is 32 in your case.

! Please add a "Comment" key to the .desktop file. This could be useful for gnome users.

(In reply to comment #1)
> * The package fails to build (also the above koji links didn't give me
> anything). 
>    File not found by glob:
> /builddir/build/BUILDROOT/gnubik-2.3-1.fc10.x86_64/usr/share/pixmap/gnubik.*
>    File not found by glob:
> /builddir/build/BUILDROOT/gnubik-2.3-1.fc10.x86_64/usr/share/man/man*/gnubik*
> 
> This line looks problematic:
>    install -Dp -m 644 doc/%{name}.6 $RPM_BUILD_ROOT%{_mandir}/man6/%{name}.6
> 
> Try to create the directory before installing the file.

Thanks. Install key -D somehow fails when building in mock.

> * As far as I know, %{_datadir}/pixmaps is being deprecated and the new
> applications should install their pixmaps under
> %{_datadir}/icons/hicolor/<dim>x<dim>/apps/
> where <dim> is 32 in your case.

Pixmaps dir is listed is seem not listed as deprecated in standard:
http://standards.freedesktop.org/icon-theme-spec/latest/ar01s03.html

> ! Please add a "Comment" key to the .desktop file. This could be useful for
> gnome users.  

Fixed.


Spec URL: http://atorkhov.fedorapeople.org/gnubik.spec
SRPM URL: http://atorkhov.fedorapeople.org/gnubik-2.3-2.fc10.src.rpm

* Wed Mar 11 2009 Alexey Torkhov <atorkhov> - 2.3-2
- Don't using install -D that doesn't want to work in mock
- Fix incorrect usage of GenericName in desktop file

(In reply to comment #2)
> (In reply to comment #1)
> 
> > * As far as I know, %{_datadir}/pixmaps is being deprecated and the new
> > applications should install their pixmaps under
> > %{_datadir}/icons/hicolor/<dim>x<dim>/apps/
> > where <dim> is 32 in your case.
> 
> Pixmaps dir is listed is seem not listed as deprecated in standard:
> http://standards.freedesktop.org/icon-theme-spec/latest/ar01s03.html
> 

No, it's not deprecated, but to my knowledge, it is _being_ deprecated. At least, comparing the size of pixmaps and hicolor directories, I think the preference in Fedora is the latter. Also, I saw packages in Fedora, where the images (that were installed by "make install") were moved from pixmaps to hicolor in the %install section. I may be wrong with this so this is by no means a blocker, you can keep images in pixmaps.

But if you use hicolor, please use the correct scriptlets from
   http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
Also, I think the image size is 32, not 48.

In addition, the %files section needs to be modified accordingly.

> > ! Please add a "Comment" key to the .desktop file. This could be useful for
> > gnome users.  
> 
> Fixed.
> 
> 

Thanks. But you didn't need to remove the GenericName. KDE makes use of GenericName, while gnome uses Comment. I think it is best to have both of them.

I will go over this package and do the full review asap.

Spec URL: http://atorkhov.fedorapeople.org/gnubik.spec
SRPM URL: http://atorkhov.fedorapeople.org/gnubik-2.3-3.fc10.src.rpm

* Wed Mar 11 2009 Alexey Torkhov <atorkhov> - 2.3-3
- Put icon into hicolor theme (the whole 10 colors! :)
- Add correct scriptlets and requires
- Add GenericName to desktop file

Okay, here's the rest of the full review: (* = blockers, ! = suggestions)

* The Release field in the above SPEC file is incorrect.

! Please expand the description to 80 columns.

* Please remove the binary .gmo files in %prep

! It would probably better to build the .info file from source with something like
   makeinfo doc/gnubik.texinfo
Note that this will need adding texinfo to BuildRequries.

! The BRs: libX11-devel mesa-libGL-devel mesa-libGLU-devel gtk2-devel are not necessary. They will be pulled up by gtkglext-devel.
(Side note: When we really need to BR mesa-libGL-devel or mesa-libGLU-devel, we should BR the virtual provides' libGL-devel libGLU-devel. But as I said above, this is unnecessay for this package)

! The explicit Requires: hicolor-icon-theme is not necessary. It will be automatically picked up by dependency chain.

   makeinfo doc/gnubik.texinfo
should be
   makeinfo doc/%{name}.texinfo
in the above comment.

Spec URL: http://atorkhov.fedorapeople.org/gnubik.spec
SRPM URL: http://atorkhov.fedorapeople.org/gnubik-2.3-4.fc10.src.rpm

(In reply to comment #5)
> Okay, here's the rest of the full review: (* = blockers, ! = suggestions)
> 
> * The Release field in the above SPEC file is incorrect.

Sorry, I put wrong version of spec.

> ! Please expand the description to 80 columns.

Fixed.

> * Please remove the binary .gmo files in %prep

Fixed.

> ! It would probably better to build the .info file from source with something
> like
>    makeinfo doc/gnubik.texinfo
> Note that this will need adding texinfo to BuildRequries.

Fixed.

> ! The BRs: libX11-devel mesa-libGL-devel mesa-libGLU-devel gtk2-devel are not
> necessary. They will be pulled up by gtkglext-devel.
> (Side note: When we really need to BR mesa-libGL-devel or mesa-libGLU-devel, we
> should BR the virtual provides' libGL-devel libGLU-devel. But as I said above,
> this is unnecessay for this package)

Those libs are explicitly checked in configure and used in code. I'd like to leave it in perfection of agreement between spec and code :)

> ! The explicit Requires: hicolor-icon-theme is not necessary. It will be
> automatically picked up by dependency chain.  

I would prefer if this dep is also listed explicitly.

Everything is good except there is one little thing that is actually my fault.

The "makeinfo doc/%{name}.texinfo" command builds the %{name}.info file in the root of the source tree, not in the doc/ directory. Moreover, if the %{name}.info file in the doc/ directory is missing the Makefile will build it, provided that texinfo is available.

So instead of using "makeinfo doc/%{name}.texinfo" in build, we just need to do "rm doc/%{name}.info" in %prep and the Makefile will take care of the rest.

This is the only change that needs to be done and again I'm sorry for the confusion. I should have been more careful. I'm approving the package now. Please correct this issue before you commit.

-----------------------------------------
This package (gnubik) is APPROVED by oget
-----------------------------------------


By the way. It took me about 10 minutes to solve the 3x3x3 cube with this game. It normally takes me about ~90 seconds on a real cube. Any tips? :)

Have more practice :)

Thanks for review!

New Package CVS Request
=======================
Package Name: gnubik
Short Description: 3D interactive graphics puzzle
Owners: atorkhov
Branches: F-9 F-10
InitialCC:

cvs done.

gnubik-2.3-5.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/gnubik-2.3-5.fc9

gnubik-2.3-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/gnubik-2.3-5.fc10

gnubik-2.3-5.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

gnubik-2.3-5.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

gnubik desktop icon is not shown.

(In reply to comment #15)
> gnubik desktop icon is not shown.  

Icon was put in wrong location. Thanks for catch!