Bug 333921 - Review Request: xgrav - A simple physics simulation for a large number of particles
Summary: Review Request: xgrav - A simple physics simulation for a large number of par...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Patrice Dumas
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-10-16 11:54 UTC by Gwyn Ciesla
Modified: 2007-11-30 22:12 UTC (History)
4 users (show)

Fixed In Version: 1.2.0-4.fc7
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-10-24 07:17:24 UTC
Type: ---
Embargoed:
pertusus: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Gwyn Ciesla 2007-10-16 11:54:14 UTC
Spec URL: http://zanoni.jcomserv.net/fedora/xgrav/xgrav.spec
SRPM URL: http://zanoni.jcomserv.net/fedora/xgrav/xgrav-1.2.0-1.fc7.src.rpm
Description: X-Grav simulates the effect of gravity, collisions, heat dissipation and a simple chemical reaction. The simulation is in no way meant to be
realistic but rather a toy with which you can create stars, planets and even simple solar systems.

Comment 1 Patrice Dumas 2007-10-16 12:53:24 UTC
Instead of patching the Makefile to add RPM_OPT_FLAGS,
I suggest overwriting LINUX_CFLAGS, like

make LINUX_CFLAGS="-c $RPM_OPT_FLAGS `pkg-config --cflags sdl` -DWITH_ROOTWINDOW"

You could also override LINUX_LDFLAGS with
LINUX_LDFLAGS="$RPM_OPT_FLAGS `pkg-config --libs sdl` -lGL `pkg-config --libs
x11` -lm"

zlib-devel isn't directly used, so if you override the LDFLAGS
as I suggest above you can remove that dependency.

Please add a comment stating where the png file comes
from.

In the .desktop file you don't need to add the .png to
the icon name (it is a suggestion).

Also it would be nice to honor rpm macros in the desktop file
and therefore do something like
sed 's;/usr/share;%_datadir;' %{SOURCE1} > xgrav.desktop
and then run desktop-file-install on xgrav.desktop.

To keep timestamps, you should use install -p in
install -p -m 644 example* %{buildroot}%{_datadir}/xgrav

You should use %{buildroot} or $RPM_BUILD_ROOT but not both.


Comment 3 Patrice Dumas 2007-10-16 21:01:14 UTC
The %build section is a bit broken. There are 2 make invocations
and LINUX_LDFLAGS is not used. I think that it should be:

make LINUX_CFLAGS="-c $RPM_OPT_FLAGS `pkg-config --cflags sdl` -DWITH_ROOTWINDOW" 
LINUX_LDFLAGS="$RPM_OPT_FLAGS `pkg-config --libs sdl` -lGL `pkg-config --libs
x11` -lm"

All on one line. I don't remember if putting the variables in 
the environment works, I think it doesn't.


The timestamp for the source file isn't kept. You can use
wget -N or spectool -g or similar curl option. Quite strangely,
the stamp is only 4 seconds different from upstream...

Source match upstream:
2214b0591ced40f27e00accdb8fa8f41  xgrav-1.2.0.tgz


Comment 4 Gwyn Ciesla 2007-10-16 21:12:04 UTC
Whoops, not enough caffiene. :)

Addressed.

Spec URL: http://zanoni.jcomserv.net/fedora/xgrav/xgrav.spec
SRPM URL: http://zanoni.jcomserv.net/fedora/xgrav/xgrav-1.2.0-3.fc7.src.rpm

Comment 5 Patrice Dumas 2007-10-16 23:07:37 UTC
%{_bindir}/xgrav is listed twice

Comment 6 Gwyn Ciesla 2007-10-17 10:09:51 UTC
Spec URL: http://zanoni.jcomserv.net/fedora/xgrav/xgrav.spec
SRPM URL: http://zanoni.jcomserv.net/fedora/xgrav/xgrav-1.2.0-4.fc7.src.rpm

Fixed.

But if the binary is built twice and listed twice, isn't the package just that
much BETTER? ;)

Comment 7 Patrice Dumas 2007-10-17 16:49:09 UTC
* rpmlint is silent
* follow guidelines
* free software license
* match upstream
2214b0591ced40f27e00accdb8fa8f41  xgrav-1.2.0.tgz
* use macros
* files section right
* build and works on devel

APPROVED

Comment 8 Gwyn Ciesla 2007-10-17 17:00:12 UTC
Thank you!

New Package CVS Request
=======================
Package Name: xgrav
Short Description: A simple physics simulation for a large number of particles
Owners: limb
Branches: FC-6 F-7 devel
InitialCC: 
Cvsextras Commits: yes

Comment 9 Marek Mahut 2007-10-17 17:09:32 UTC
What release note do you want to include and why?

Comment 10 Marek Mahut 2007-10-17 17:11:59 UTC
Oh, I guess you wanted to request CVS flag and not release note. 

Comment 11 Gwyn Ciesla 2007-10-17 17:15:09 UTC
Yeah, sorry, finger failure. :)

Comment 12 Kevin Fenzi 2007-10-18 00:18:42 UTC
cvs done.

Comment 13 Gwyn Ciesla 2007-10-18 11:55:37 UTC
Imported and built.

Comment 14 Fedora Update System 2007-10-24 07:17:22 UTC
xgrav-1.2.0-4.fc7 has been pushed to the Fedora 7 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 15 Gwyn Ciesla 2007-11-08 19:47:39 UTC
Package Change Request
======================
Package Name: xgrav
New Branches: F-8

Missed it at the mass branching, or maybe it was too close to the freeze and I
didn't request it.

Comment 16 Kevin Fenzi 2007-11-10 20:34:43 UTC
cvs done.


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