Bug 228425 (gtkpod-review)

Summary: Review Request: gtkpod - Graphical song management program for Apple's iPod
Product: [Fedora] Fedora Reporter: Todd Zullinger <tmz>
Component: Package ReviewAssignee: Xavier Lamien <lxtnow>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideKeywords: Reopened
Target Milestone: ---Flags: lxtnow: fedora-review+
wtogami: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-03-02 16:23:15 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 163779    

Description Todd Zullinger 2007-02-13 02:18:16 UTC
Spec URL: http://pobox.com/~tmz/fedora/gtkpod-0.99.8-1.fc6-result/gtkpod.spec
SRPM URL: http://pobox.com/~tmz/fedora/gtkpod-0.99.8-1.fc6-result/gtkpod-0.99.8-1.fc6.src.rpm
Description: gtkpod is a platform independent Graphical User Interface for Apple's
iPod using GTK2. It supports the first to fifth Generation including
the iPod mini, iPod Photo, iPod Shuffle, iPod nano, and iPod Video.

(Mock logs and i386 packages are also available at http://pobox.com/~tmz/fedora/gtkpod-0.99.8-1.fc6-result/)

Comment 1 Xavier Lamien 2007-02-13 04:40:18 UTC
--------
BuildRequires
--------
* gtk2-devel for BuildRequires is redundant. libglade2-devel
  requires gtk2-devel.

--------
%prep
--------
* You should comment why you apply a patch.
* Desktop file:
  personnaly, i don't like to use cat << eof to create desktop file.
  I think it's better to add desktop file as Source file.
  "X-livna" must be removed from categorie.

---------
%build
---------
looks good

---------
%install
---------
* Please use install -Dm 644 instead of %{__install} -D -m 0644
* Same thing for %{__mkdir_p}.
* in desktop-file-intall entry:
  use of "--vendor livna" is deprecated and must be removed, 
  just use --vendor ""
  Also add --mode 0644

---------
Scriptlet
---------
you should make use of GTK+ icon cache
See :
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?action=show&redirect=ScriptletSnippets#head-7103f6c38d1b5735e8477bdd569ad73ea2c49bda

%post
touch --no-create %{_datadir}/icons/hicolor || :
%{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor || :

%postun
touch --no-create %{_datadir}/icons/hicolor || :
%{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor ||

--------
%files
--------
The file NEWS is useless, you can drop it as its contents refers to ChangeLog

--------

Rpmlint isn't silent from srpm file:
          -- E: gtkpod unknown-key PGP#beaf0ce3 --

SOULDN'T: Packages must not be sign with your own key


Comment 2 Todd Zullinger 2007-02-13 05:45:32 UTC
Hi Xavier,

> * gtk2-devel for BuildRequires is redundant. libglade2-devel
>   requires gtk2-devel.

Fixed.

> * You should comment why you apply a patch.

Okay.  Done.  In this case the patch is from upstream and is required to build
against libgpod-0.4.2.

> * Desktop file:
>   personnaly, i don't like to use cat << eof to create desktop file.
>   I think it's better to add desktop file as Source file.

Done.

>   "X-livna" must be removed from categorie.

Egad, that's an embarrassing copy and paste error on my part (I was working on
an update to gtkpod for livna).

Fixed.

> * Please use install -Dm 644 instead of %{__install} -D -m 0644
> * Same thing for %{__mkdir_p}.

Done.

> * in desktop-file-intall entry:
>   use of "--vendor livna" is deprecated and must be removed, 
>   just use --vendor ""

Shouldn't it be fedora instead?  The guidlines say "If upstream uses
<vendor_id>, leave it intact, otherwise use fedora as <vendor_id>."

I used fedora in the latest spec.

>   Also add --mode 0644

Done.  (Is that really needed?  It's not in the guidelines and the file in a
built package are the proper mode.  I'm curious now. :)

> you should make use of GTK+ icon cache
> See :
>
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?action=show&redirect=ScriptletSnippets#head-7103f6c38d1b5735e8477bdd569ad73ea2c49bda
> 
> %post
> touch --no-create %{_datadir}/icons/hicolor || :
> %{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor || :
> 
> %postun
> touch --no-create %{_datadir}/icons/hicolor || :
> %{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor ||

The icon isn't installed into %{_datadir}/icons/hicolor but into
%{_datadir}/pixmaps.  I didn't think that was relevant then.  Or are you
suggesting that I install the icon under %{_datadir}/icons/hicolor?

> The file NEWS is useless, you can drop it as its contents refers to ChangeLog

Done.

> Rpmlint isn't silent from srpm file:
>           -- E: gtkpod unknown-key PGP#beaf0ce3 --
> 
> SOULDN'T: Packages must not be sign with your own key

Sorry, I'm a bit of a crypto geek and I sign things a lot.

Here are the updated files:

Spec URL: http://pobox.com/~tmz/fedora/gtkpod/gtkpod.spec
SRPM URL: http://pobox.com/~tmz/fedora/gtkpod/gtkpod-0.99.8-2.fc6.src.rpm

Comment 3 Xavier Lamien 2007-02-16 01:35:32 UTC
* "cp %{SOURCE1} %{name}.desktop" from %prep section is useless.
  you can directly use %{SOURCE1} in desktop-file-install entry.

> Shouldn't it be fedora instead?  The guidlines say "If upstream uses
> <vendor_id>, leave it intact, otherwise use fedora as <vendor_id>."
> I used fedora in the latest spec.

vendor id is not require anymore.
resume from guilines :
[ # The Vendor tag should not be used. It is set automatically by the build
system. ]

* You use an 48x48 icon, and i think it maybe souhld be install in
%{_datadir}/icons/
* Also add -p option to install (install -p -Dm 644 /.././)

* You should add timestamp to make install:
make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"



Comment 4 Xavier Lamien 2007-02-16 01:37:10 UTC
s/quilines/guidelines

Comment 5 Todd Zullinger 2007-02-16 04:53:09 UTC
Hi Xavier,

> * "cp %{SOURCE1} %{name}.desktop" from %prep section is useless.
>   you can directly use %{SOURCE1} in desktop-file-install entry.

I realize that.  It is more legible to me to have the cp in %prep as it is
easier in the editor to see what file %{SOURCE1} is referencing.  Also it puts
it in the builddir so that if someone does a %prep and then checks out the
builddir they will see the file there.

> vendor id is not require anymore.
> resume from guilines :
> [ # The Vendor tag should not be used. It is set automatically by the build
> system. ]

That quote is from the section on spec file tags.  It is for a Vendor: tag, not
for the vendor option to desktop-file-install.  See the section on .desktop
files: http://fedoraproject.org/wiki/Packaging/Guidelines#desktop

> * You use an 48x48 icon, and i think it maybe souhld be install in
> %{_datadir}/icons/

I thought that %{_datadir}/icons/ was for themeable icons and such.  I know
there are lots of packages from core and extras that install icons in
%{_datadir}/pixmaps.  I don't see anything in the guidelines about this.  Do
you know where I can find discussion or current best practices on this?

> * You should add timestamp to make install:
> make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"

I have never seen that required or recommended before.  Could you tell me why
you feel it is needed and where it is discussed or recommended?  I use the
install line straight from the specfile template in rpmdevtools.

Comment 6 Mamoru TASAKA 2007-02-16 05:18:10 UTC
(In reply to comment #5)
 
> > * You should add timestamp to make install:
> > make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"
> 
> I have never seen that required or recommended before.  Could you tell me why
> you feel it is needed and where it is discussed or recommended?  I use the
> install line straight from the specfile template in rpmdevtools.

Well, first check the "Timestamps" of
http://fedoraproject.org/wiki/Packaging/Guidelines

Keeping timestamps on the files as much as possible makes
it easier to check:
* if the vendor (like you) has modified the original file
* and when the files are created or modified
and
* this may avoid the creation of .rpmsave .rpmnew files
  when it is unnecessary

So keeping timestamps is recommended, especially on documentations,
image files, configuration files, ...

Well, the ways to keep timestamps on the files automatically installed
by "make install" differ according to packages, however,
in general cases "make INSTALL="install -p" install" works.
(there are many cases in that more fixes are needed). 

Comment 7 Todd Zullinger 2007-02-16 06:11:54 UTC
(In reply to comment #6)
> Well, first check the "Timestamps" of
> http://fedoraproject.org/wiki/Packaging/Guidelines

Got it.  I'll add -p to the copy and installation of the icon.  Though neither
of them are config files so they shouldn't ever generate any sort of
.rpm{new,save} files.  And in the case of the .destkop file the timestamp of the
installed file will be whatever time the build was run as desktop-file-install
will update it.  (I'm now thinking I should have just kept the .desktop
generation in the spec file.)

> Keeping timestamps on the files as much as possible makes
> it easier to check:
> * if the vendor (like you) has modified the original file
> * and when the files are created or modified

The file will also be in cvs, so checking such things is always possible. :)

> Well, the ways to keep timestamps on the files automatically installed
> by "make install" differ according to packages, however,
> in general cases "make INSTALL="install -p" install" works.
> (there are many cases in that more fixes are needed). 

I've not run across any specfiles that do this.  It's not in the guidelines that
I can find (nor on the wiki anyplace, though my searching may just not have
found it).  I'd really like to see the pros and cons (if there are any)
discussed more.

I'll add it and test a little bit to see that it works as it should.  If either
of you has some pointers to places this has been discussed previously, please
pass them along.  My only concern is that I'll find out later that this can
cause some problems that I'm not aware of.

Comment 8 Todd Zullinger 2007-02-16 07:34:47 UTC
Perhaps instead of using install for the icon, a symlink is better?  Like so:

# install symlink to menu icon
mkdir -p %{buildroot}%{_datadir}/pixmaps
ln -s ../%{name}/pixmaps/%{name}-icon-48.png \
    %{buildroot}%{_datadir}/pixmaps/%{name}.png

This saves on duplication of the icon file.

Comment 9 Xavier Lamien 2007-02-18 00:44:05 UTC
Doesn't sound good. Just change path in desktop entry to point to
%{_datadir}/icons/ directory.

Comment 10 Todd Zullinger 2007-02-18 04:29:28 UTC
Xavier, would you please show me where this is required or recommended?  Just
saying I need to do something isn't very good in my opinion.  I want to know why
it should be changed so that I understand it for next time.  This will also help
others reading the review to learn why one path is better than another.

Better than that, if I understand why it's better in one place than the other I
can most likely get it installed there upstream so I don't have to worry about
doing it in the specfile.

A quick grep of the specfiles in extras-development shows me there are 63 specs
that install something to %{_datadir}/pixmaps and 39 installing to
%{_datadir}/icons.  It seems to me that unless there is a requirement to use one
or the other, that this is just a matter of preference and it should not be a
blocker.  If you know differently or have good reason why I should install under
%{_datadir}/icons, please enlighten me.  I am always insterested in learning.

No matter where it ends up going, since the file is one that's included with the
upstream tarball and is already installed in %{_datadir}/%{name}/pixmaps, I
don't see why a symlink isn't better than a copy of the file.  Please explain
why a symlink isn't any good to you.

Thanks.

Comment 11 Xavier Lamien 2007-02-20 23:29:36 UTC
The use of %{_datadir}/pixmaps location isn't a blocker and surely not a preference.

my sentence was : 
* You use an 48x48 icon, and i think it **maybe** souhld be install in
%{_datadir}/icons/

And if you use to install the icon in %{_datadir}/icons location, the symlink is
not recommanded.
So, you "icon=" tag in .desktop file should be point to %{_datadir}/icon/



Comment 12 Todd Zullinger 2007-02-21 07:44:28 UTC
AFAIK, there's no need to specify the full path in the .desktop file.  The
freedesktop.org Icon Theme Specification[1] covers what directories will be
searched.  I'll try to get the .desktop file and icons installed according to
the fdo specs in a future release.  For now I think a symlink in
%{_datadir}/pixmaps works fine.

I've uploaded a spec and srpm that incorporates the changes which I think are
sufficient.  Let me know if there are any issues that remain before the package
is approved.  Thanks!

Spec URL: http://pobox.com/~tmz/fedora/gtkpod/gtkpod.spec
SRPM URL: http://pobox.com/~tmz/fedora/gtkpod/gtkpod-0.99.8-3.fc7.src.rpm

[1] http://standards.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html

Comment 13 Xavier Lamien 2007-02-21 16:06:19 UTC
Well,

After had a look on the spec file, all things seem good,
except the --vendor tag which shouldn't be set to "fedora".

I'm plan to modify the packaging Guidlines's page to notify that stuff.

I'll make a full review within a day.

Comment 14 Todd Zullinger 2007-02-21 16:18:17 UTC
So the section in the guidelines on using the vendor option to
desktop-file-install is incorrect?  Are you sure?  The section you linked to
previously wasn't related to desktop-file-install but instead to Vendor: tags in
the specfile header.  Do you think you could help me out with a link to any
discussion on where this was changed?  I could easily have missed it if the
packaging committee recently changed this.

Thanks.

Comment 15 Todd Zullinger 2007-02-22 15:54:17 UTC
I asked on the fedora-packaging list[1] if the use of vendor when installing
.desktop files had changed recently.  It hasn't.  The guidelines have been
updated recently to clarify other things about installing .desktop files but
vendor is still needed if upstream doesn't use it[2].

[1] https://www.redhat.com/archives/fedora-packaging/2007-February/msg00206.html
[2] http://fedoraproject.org/wiki/Packaging/Guidelines#desktop

Comment 16 Xavier Lamien 2007-02-23 04:01:25 UTC
The fact is, the vendor id is automaticaly genereted.

I'll discuss with Rdieter about that.

As you can see in Guidelines page, there's not mention about the use of "--add
categorie=" which's deprecated in spec file.

Comment 17 Todd Zullinger 2007-02-23 05:00:04 UTC
No, vendor is not automatically generated for .desktop files.  Where did you get
this info (a link would be appreciated)?  The guidelines are quite clear on the
usage and the reasoning for it.  I think you must be confused about adding a
Vendor: tag/header to the specfile.  That (Vendor:) isn't required and indeed is
generated.

The vendor option to desktop-file-install is not.  Please edit the specfile to
remove the vendor option and try to build in mock.  You'll see an error like the
following at the desktop-file-install command:

+ desktop-file-install --mode 0644 --dir
/var/tmp/gtkpod-0.99.8-3.fc6-root-mockbuild/usr/share/applications gtkpod.desktop
Must specify the vendor namespace for these files with --vendor

I did this with FC6 as the target, but I have no reason to expect it to be
different in development.  If you find that it is, please let me know.

I'm not sure what you mean about "--add-categorie" - I don't have that in the
spec file.  AFAIK, there are times when adding or removing categories from an
upstream .desktop file are needed, but that's irrelevant to this review.

If you want to get clarification on this, please do so in the thread on
fedora-packaging that I started so that I (and others) can be included in the
discussion.

Is there anything else that's holding up approval of this package?

Comment 18 Todd Zullinger 2007-02-23 07:09:39 UTC
(In reply to comment #17)
> I did this with FC6 as the target, but I have no reason to expect it to be
> different in development.  If you find that it is, please let me know.

I just did a mock build against development and desktop-file-install there
doesn't error, so it sit corrected.

However, without the vendor tag the installed .desktop file is named
gtkpod.desktop and not fedora-gtkpod.desktop as it would be with the --vendor
option set.  This is what I understand the guidelines to be concerned about: the
changing name of the desktop file (and its effect on menu editors).

I intend to build gtkpod for both FC6 and development and I don't think they
should use different names for the .desktop file.  As always, if I'm missing
something obvious or something that's been changed, please point me to it so I
can learn.

Comment 19 Xavier Lamien 2007-02-23 13:03:00 UTC
Review:

OK package builds in mock (FC-5 FC-6 FC-Devel i386,x86_64).
OK rpmlint is silent for both SRPM and RPM.
OK source files match upstream.
OK package meets naming and packaging guidelines.
OK specfile is properly named, is cleanly written
OK Spec file is written in American English.
OK Spec file is legible.
OK dist tag is present.
OK build root is correct.
OK license is open source-compatible.
OK License text is included in package.
OK %doc is good
OK %doc does not affect runtime.
OK BuildRequires are proper.
+  Vendor is set to Fedora.
OK %clean is present.
OK package installed properly.
OK Macro use appears rather consistent.
OK Package contains code, not content.
OK no static libraries.
OK no translations are available.
OK Does owns the directories it creates.
OK no duplicates in %files.
OK Changelog is present and clean.
OK file permissions are appropriate.

APPROVED


Comment 20 Xavier Lamien 2007-02-23 13:09:14 UTC
for the rest, we will discuss about that in the ML ;-)

already sponsored ?


Comment 21 Todd Zullinger 2007-02-23 16:08:19 UTC
(In reply to comment #19)
> OK package builds in mock (FC-5 FC-6 FC-Devel i386,x86_64).

Actually, it won't build for FC-5 because libgpod >= 0.4.2 is required and that
is not available for FC5 (and won't be, unfortunately - see BZ#196876).

Thanks for checking it on x86_64.  I don't have access to anything but i386
currently.

> APPROVED

Thanks.

(In reply to comment #20)
> for the rest, we will discuss about that in the ML ;-)

Sounds good.

> already sponsored ?

Yep.  Thanks for the review.

Comment 22 Todd Zullinger 2007-02-23 16:21:15 UTC
New Package CVS Request
=======================
Package Name: gtkpod
Short Description: Graphical song management program for Apple's iPod
Owners: tmz
Branches: FC-6 devel
InitialCC:

Comment 23 Xavier Lamien 2007-02-28 18:25:59 UTC
Don't forget to CLOSE this bug and set resolultion to NEXTRELEASE after imported
and built your package from cvs.

Comment 24 Todd Zullinger 2007-02-28 18:49:21 UTC
I haven't forgotten.  I've built for devel already (only showed up recently due
to the devel freeze).  The FC-6 branch is now in needsign state.  I'll close
this after that build gets pushed.