Bug 249992

I do have some queries about the compile process:
gcc -Wall -O3 -I/usr/X11/include   -c -o vmath.o vmath.c
gcc -Wall -O3 -I/usr/X11/include   -c -o globe.o globe.c
globe.c: In function 'make_globe_old2':
globe.c:507: warning: dereferencing type-punned pointer will break
strict-aliasing rules
globe.c:511: warning: pointer targets in assignment differ in signedness
globe.c: In function 'make_globe':
globe.c:663: warning: dereferencing type-punned pointer will break
strict-aliasing rules
globe.c:665: warning: dereferencing type-punned pointer will break
strict-aliasing rules

Should I be concerned about these warnings ? Should I attempt to patch around them ?
Thanks, DaveT.

I wouldn't worry overmuch about the warnings, but I would worry quite a bit
about the fact that your package doesn't seem to honor %{optflags}.  You'll need
to figure out how to pass the proper compiler flags.

Also, to answer your question about commenting out %configure, % macros are
still expanded if the line begins with '#'.  Either double the '%' or remove it.

Jason, is that dis-honouring caused by removing the %configure ?
Did you determine that it doesn't by the params passed to gcc ? {Hoping for a
hint on where to look} Will it be the source's make file ? Can the package patch
the make file to solve it ?

commented macros:
Is it because the macros expanded out are multi-line, and perhaps even start
with a new line that the expanded part does not stay commented ?

An offside: The upstream developer hasn't touched the source in years, from what
I can tell. I remember both Hans and yourself mentioned on fedora-devel that
non-activity is not necessarily a blocker. In any case, I have requested of
Florian whether he has/can make some nicer icons.

ps. This is my first package up for review, and I'll eventually need a sponsor.

I did not investigate how this package builds, so at this point I can't tell you
how to pass the proper compiler flags to it.  I don't see any configure script
in the package, so I'm not sure what good a call to %configure could possibly
do.  If there's just a plain Makefile, you probably just need check that it will
allow you to pass CFLAGS on the make line, or patch it to allow that.

> Is it because the macros expanded out are multi-line, and perhaps even start
> with a new line that the expanded part does not stay commented ?

Perhaps; I'm not intimately familiar with the details of how RPM expands macros.

> ps. This is my first package up for review, and I'll eventually need a
> sponsor.

This should block FE-NEEDSPONSOR, then.

Updated spec: http://members.iinet.net.au/~timmsy/glglobe/glglobe.spec
Updated src.rpm:
http://members.iinet.net.au/~timmsy/glglobe/glglobe-0.2-2.fc7.src.rpm

- del commenting about non-existent %configure.
- add export CFLAGS="$RPM_OPT_FLAGS" so that rpm flags are used during build.
- add makefile patch to use environment CFLAGS if they are defined.

Current spec: http://members.iinet.net.au/~timmsy/glglobe/glglobe.spec
Updated src.rpm:
http://members.iinet.net.au/~timmsy/glglobe/glglobe-0.2-3.fc7.src.rpm

It didn't build in mock due to desktop-file-install not being available:
- add BuildRequires: desktop-file-utils so that it build in mock

Well, for 0.2-3:

* Source1-4
  - How did these created? If you downloaded from some URLs,
    please write the full URLs like Source1.
    Otherwise please write comments how you created these
    files.

* Desktop file
  - Categories "X-Red-Hat-Extra X-Red-Hat-Base" are both deprecated
    and should be removed.

* Requires for scriptlets
--------------------------------------------------
Requires(post):   desktop-file-utils
Requires(postun): desktop-file-utils
--------------------------------------------------
  - are not needed (actually no scriptlets require 
    desktop-file-utils ).

* Documents
--------------------------------------------------
%install
....
mkdir -p %{buildroot}%{_docdir}/glglobe
install -p -m 0644 ChangeLog COPYING README TODO %{buildroot}%{_docdir}/glglobe
....
%files
%doc %{_datadir}/doc/*
--------------------------------------------------
  - Usually documents must be version specific. Just use:
--------------------------------------------------
%doc ChangeLog COPYING README TODO
--------------------------------------------------
    (and check what actually happens).

* desktop-file-utils usage
--------------------------------------------------
install -p -m 0644 %{SOURCE4} %{buildroot}%{_datadir}/applications
desktop-file-install --vendor fedora --delete-original \
  --dir %{buildroot}%{_datadir}/applications           \
  --add-category X-Red-Hat-Base                        \
  %{buildroot}%{_datadir}/applications/glglobe.desktop
----------------------------------------------------
   - This can be replaced by
----------------------------------------------------
desktop-file-install --vendor fedora \
   --dir %{buildroot}%{_datadir}/applications \
   %{SOURCE4}
----------------------------------------------------
   (and remove deprecated categories)

* Directory ownership
  - The directory %{_datadir}/glglobe/ is not owned by any packages
  - The directories 
--------------------------------------------------
%{_datadir}/icons/hicolor
%{_datadir}/icons/hicolor/*
%{_datadir}/icons/hicolor/*/apps
--------------------------------------------------
    are already owned by hicolor-icon-theme and this package
    must not own these.

One more issue
* License
  - License tag policy is changed and now you (we) must follow:
    http://fedoraproject.org/wiki/Packaging/LicensingGuidelines and
    http://fedoraproject.org/wiki/Licensing

    As far as I checked the source code, this is licensed under
    GPL version 2 only. So please use "GPLv2" as license tag.

Mamoru, thanks for taking a look:

(In reply to comment #7)
Updated spec: http://members.iinet.net.au/~timmsy/glglobe/glglobe.spec
SRPM: http://members.iinet.net.au/~timmsy/glglobe/glglobe-0.2-4.fc7.src.rpm

- add comment re Source1..4 creation
- mod .desktop category to Education, although some or all of the {Education,
Graphics, DataVisualization, Geography, Amusement, ImageProcessing } might fit ?
- del .desktop category deprecated items
- del extraneous BR and Requires(post/postun) for desktop-file-utils
> desktop-file-install --vendor fedora --delete-original \
When I followed other packages as an example, it seems that --delete-original
was normal. Is this a changed packaging standard ?
- mod desktop-file-install to simpler command, using SOURCEx
- add ownership of app data directory
- mod ownership of icon data to glglobe specific filenames

(In reply to comment #8)
- mod License to meet new Licensing Guidelines
Changed but didn't work out rpmlint's unhappiness until I realized I hadn't
updated rpmlint on that machine ;)

Current spec: http://members.iinet.net.au/~timmsy/glglobe/glglobe.spec
Updated SRPM:
http://members.iinet.net.au/~timmsy/glglobe/glglobe-0.2-4.fc7.src.rpm

A query: is it worth testing in fc6 and fdevel {my testing has been in f7}?

I have not checked -4 spec/srpm, for now just answering your
comment:

(In reply to comment #9)
> - mod .desktop category to Education, although some or all of the {Education,
> Graphics, DataVisualization, Geography, Amusement, ImageProcessing } might fit ?
  - IMO only one Category "Education" is enough.

> > desktop-file-install --vendor fedora --delete-original \
> When I followed other packages as an example, it seems that --delete-original
> was normal. Is this a changed packaging standard ?
  - You don't have to (and must not) delete the original SOURCEx.
    The reason you had to use --delete-original was if you didn't
    use it, i.e.
--------------------------------------------------
install -p -m 0644 %{SOURCE4} %{buildroot}%{_datadir}/applications
desktop-file-install --vendor fedora  \
  --dir %{buildroot}%{_datadir}/applications           \
  --add-category X-Red-Hat-Base                        \
  %{buildroot}%{_datadir}/applications/glglobe.desktop
----------------------------------------------------
    This result in leaving 2 desktop files named "glglobe.desktop"
    and "fedora-glglobe.desktop" under %{buildroot}%{_datadir}/applications .
    So in this usage --delete-original was actually needed.

    By the new usage (written below:)
----------------------------------------------------
desktop-file-install --vendor fedora \
    --dir %{buildroot}%{_datadir}/applications %{SOURCE4}
----------------------------------------------------
    Only one desktop file named "fedora-glglobe.desktop" is created
    under %{buildroot}%{_datadir}/applications so it is already
    okay. And you don't have to (and must not) delete the original
    SOURCE4 file.
 
> A query: is it worth testing in fc6 and fdevel {my testing has been in f7}?
- If you have FC6 or rawhide machine and you can test on them,
  of course it is appreciated.

Well, for -4:
* Desktop category
  Category "X-Red-Hat-Extra" is still there, which is not needed.

Then:
-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------

(In reply to comment #11)
> Well, for -4:
> * Desktop category
>   Category "X-Red-Hat-Extra" is still there, which is not needed.
Oops, I forgot to cp the updated .desktop to the SOURCES directory, so it built
with the earlier version.

fix noted in spec: http://members.iinet.net.au/~timmsy/glglobe/glglobe.spec
Updated SRPM: 
http://members.iinet.net.au/~timmsy/glglobe/glglobe-0.2-5.fc7.src.rpm

It builds in f7 mock. It installs and runs on a minimal noX f7 install
{forwarding the X connection to my normal gui desktop}, so it seems rpm is
picking up the correct runtime libs - but is that a reasonable way to check
Requires ?
=====
Currently I am pre-reviewing:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=234326
and initial looks at:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=251019
with a more complete pre-review to come.

(In reply to comment #12)
> It builds in f7 mock. It installs and runs on a minimal noX f7 install
> {forwarding the X connection to my normal gui desktop}, so it seems rpm is
> picking up the correct runtime libs - but is that a reasonable way to check
> Requires ?
  It depends on cases. This (glglobe) is a single package with no
  subpackages and dependency check is rather easy.
  If one srpm creates some subpackages the dependencies between those
  subpackages should be carefully examined. Also there are some
  dependencies which rpmbuild does not detect automatically..
  So actually it depends on cases...

> =====
> Currently I am pre-reviewing:
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=234326
  - Please check my comment.
    * If you do a (pre)review, please write clearly what you think
      is blocking the review

However I will accept this review
-------------------------------------------------------------
    This package (glglobe) is APPROVED by me
-------------------------------------------------------------
Please follow the procedure according to:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
At a point a mail should be sent to sponsor members which notifies
that you need a sponsor (at the stage, please also write on
this bug for confirmation that you requested for sponsorship)
Then I will sponsor you.

If you want to import this package into Fedora 7, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on Fedora rebuilding system).

If you have questions, please ask me.

As described in the /Join link, I am confirming that I have requested membership
in the cvsextras and fedorabugs groups as described in the /Join link.

Now I am sponsoring you.

Thanks for the review and sponsoring me, Mamoru.

New Package CVS Request
=======================
Package Name: glglobe
Short Description: OpenGl Globe - Earth simulation for linux
Owners: dtimms
Branches: FC-6 F-7
InitialCC: 
Cvsextras Commits: yes

cvs done.

Thanks Kevin.
101193 build (dist-f8, devel:glglobe-0_2-5_fc8) completed successfully
Closing.

Package Change Request
======================
Package Name: glglobe
New Branches: EL-5 EL-6
Owners: dtimms

CVS done (by process-cvs-requests.py).