Bug 488124 - Review Request: gnubik - 3D interactive graphics puzzle
Review Request: gnubik - 3D interactive graphics puzzle
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-02 14:48 EST by Alexey Torkhov
Modified: 2009-03-21 15:50 EDT (History)
4 users (show)

See Also:
Fixed In Version: 2.3-5.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-13 14:37:00 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oget.fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Alexey Torkhov 2009-03-02 14:48:16 EST
Spec URL: http://atorkhov.fedorapeople.org/gnubik.spec
SRPM URL: http://atorkhov.fedorapeople.org/gnubik-2.3-1.fc10.src.rpm
Description:
GNUbik is a GNU package.  It is a 3D interactive graphics
puzzle. It renders an image of a magic cube
(similar to a rubik cube) and you attempt to solve it.

Rpmlint output clean.

Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1214325
Comment 1 Orcan Ogetbil 2009-03-11 11:35:10 EDT
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.
Comment 2 Alexey Torkhov 2009-03-11 12:34:27 EDT
(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@gmail.com> - 2.3-2
- Don't using install -D that doesn't want to work in mock
- Fix incorrect usage of GenericName in desktop file
Comment 3 Orcan Ogetbil 2009-03-11 12:48:17 EDT
(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.
Comment 4 Alexey Torkhov 2009-03-11 13:10:34 EDT
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@gmail.com> - 2.3-3
- Put icon into hicolor theme (the whole 10 colors! :)
- Add correct scriptlets and requires
- Add GenericName to desktop file
Comment 5 Orcan Ogetbil 2009-03-11 16:28:08 EDT
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.
Comment 6 Orcan Ogetbil 2009-03-11 16:29:43 EDT
   makeinfo doc/gnubik.texinfo
should be
   makeinfo doc/%{name}.texinfo
in the above comment.
Comment 7 Alexey Torkhov 2009-03-11 18:07:44 EDT
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.
Comment 8 Orcan Ogetbil 2009-03-11 22:31:21 EDT
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? :)
Comment 9 Alexey Torkhov 2009-03-12 00:44:36 EDT
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:
Comment 10 Kevin Fenzi 2009-03-12 22:48:47 EDT
cvs done.
Comment 11 Fedora Update System 2009-03-13 05:09:54 EDT
gnubik-2.3-5.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/gnubik-2.3-5.fc9
Comment 12 Fedora Update System 2009-03-13 05:10:26 EDT
gnubik-2.3-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/gnubik-2.3-5.fc10
Comment 13 Fedora Update System 2009-03-13 14:36:53 EDT
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.
Comment 14 Fedora Update System 2009-03-13 14:39:02 EDT
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.
Comment 15 nucleo 2009-03-21 15:31:05 EDT
gnubik desktop icon is not shown.
Comment 16 Alexey Torkhov 2009-03-21 15:50:19 EDT
(In reply to comment #15)
> gnubik desktop icon is not shown.  

Icon was put in wrong location. Thanks for catch!

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