Bug 514351

I'm not qualified to do a review but here are some informal comments:

1: The Source0 URL got me a 404, but I found a link to http://archive.xfce.org/src/panel-plugins/xfce4-stopwatch-plugin/0.2/xfce4-stopwatch-plugin-0.2.0.tar.bz2 on the website. The difference is ".x". Has the URL changed?

2: desktop-file-install or desktop-file-validate must be used. (https://fedoraproject.org/wiki/Packaging/Guidelines#desktop-file-install_usage) Do you have a very good reason not to do this?

3: RPMlint says:
xfce4-stopwatch-plugin.x86_64: W: incoherent-version-in-changelog 0.7.2-1 ['0.2.0-1.fc11', '0.2.0-1']
The spec in the source RPM has the wrong version number in the changelog. In the separate spec it's correct.

4: It's not exactly a serious problem but there's a typo in the changelog. It says "Fedpra".

(In reply to comment #1)
> I'm not qualified to do a review but here are some informal comments:
> 
> 1: The Source0 URL got me a 404, but I found a link to
> http://archive.xfce.org/src/panel-plugins/xfce4-stopwatch-plugin/0.2/xfce4-stopwatch-plugin-0.2.0.tar.bz2
> on the website. The difference is ".x". Has the URL changed?

Yes, this was a bug in the goodies release manager webapp, that Jannis fixed after I rolled this package.

> 2: desktop-file-install or desktop-file-validate must be used.
> (https://fedoraproject.org/wiki/Packaging/Guidelines#desktop-file-install_usage)
> Do you have a very good reason not to do this?

Yes. All the xfce4-panel plugins have
  Type=X-XFCE-PanelPlugin
instead of
  Type=Application
Although this is a valid extension of the freedesktop.org specs, desktop-file-utils refuse to install or verify the files.

> 3: RPMlint says:
> xfce4-stopwatch-plugin.x86_64: W: incoherent-version-in-changelog 0.7.2-1
> ['0.2.0-1.fc11', '0.2.0-1']
> The spec in the source RPM has the wrong version number in the changelog. In
> the separate spec it's correct.
> 
> 4: It's not exactly a serious problem but there's a typo in the changelog. It
> says "Fedpra".  

Thanks for catching these.

Updated files:
http://cwickert.fedorapeople.org/review/xfce4-stopwatch-plugin.spec
http://cwickert.fedorapeople.org/review/xfce4-stopwatch-plugin-0.2.0-2.fc12.src.rpm

(In reply to comment #2)
> > 2: desktop-file-install or desktop-file-validate must be used.
> > (https://fedoraproject.org/wiki/Packaging/Guidelines#desktop-file-install_usage)
> > Do you have a very good reason not to do this?
> 
> Yes. All the xfce4-panel plugins have
>   Type=X-XFCE-PanelPlugin
> instead of
>   Type=Application
> Although this is a valid extension of the freedesktop.org specs,
> desktop-file-utils refuse to install or verify the files.

OK. I suppose XFCE packagers know this already but for the rest of us a comment explaining that would be nice.

I will try to keep that in mind next time. I could argue that the desktop file guidelines do not apply to these desktop files at all, because a GUI app is defined as "any application which draws an X window and runs from within that window". Panel plugins are not running from their own windows but from the panel.

We also have desktop files in /usr/share/xsessions, which can not be installed with desktop-file-utils ether. Maybe someone needs to update the packaging guidelines to make the difference more obvious.

I'll take this for review.

Setting the fedora-review flag as that seems to have been overlooked.

> We also have desktop files in /usr/share/xsessions, which can not be installed
> with desktop-file-utils ether. Maybe someone needs to update the packaging
> guidelines to make the difference more obvious.  

Agreed. I've run into this issue with moblin packages too.

Just two minor notes at the bottom. One being the already acknowledged .desktop file issue. Do you know if its been bought up with the packaging committee?

+ rpmlint output

rpmlint xfce4-stopwatch-plugin.spec xfce4-stopwatch-plugin-0.2.0-2.fc11.src.rpm xfce4-stopwatch-plugin-*x86_64.rpm
3 packages and 1 specfiles checked; 0 errors, 0 warnings.

+ package name satisfies the packaging naming guidelines
+ specfile name matches the package base name
+ package should satisfy packaging guidelines
+ license meets guidelines and is acceptable to Fedora
+ license matches the actual package license
  BSD
+ %doc includes license file
+ spec file written in American English
+ spec file is legible
+ upstream sources match sources in the srpm
  2ce672f294ee42cdc49ddbe1a1d8ae96  xfce4-stopwatch-plugin-0.2.0.tar.bz2

+ package successfully builds on at least one architecture
  tested using koji scratch build
  http://koji.fedoraproject.org/koji/taskinfo?taskID=1597960
+ BuildRequires list all build dependencies
n/a %find_lang instead of %{_datadir}/locale/*
n/a binary RPM with shared library files must call ldconfig in %post and %postun+ does not use Prefix: /usr
n/a package owns all directories it creates
n/a no duplicate files in %files
+ Package perserves timestamps on install
+ %defattr line
+ %clean contains rm -rf $RPM_BUILD_ROOT
+ consistent use of macros
+ package must contain code or permissible content
n/a large documentation files should go in -doc subpackage
+ files marked %doc should not affect package
n/a header files should be in -devel
n/a static libraries should be in -static
n/a packages containing pkgconfig (.pc) files need 'Requires: pkgconfig'
n/a libfoo.so must go in -devel
n/a devel must require the fully versioned base
+ packages should not contain libtool .la files
+ packages containing GUI apps must include %{name}.desktop file
+ packages must not own files or directories owned by other packages
+ %install must start with rm -rf %{buildroot} etc.
+ filenames must be valid UTF-8

Optional:

n/a if there is no license file, packager should query upstream
n/a translations of description and summary for non-English languages, if
available
+ reviewer should build the package in mock/koji
n/a the package should build into binary RPMs on all supported architectures
n/a review should test the package functions as described
+ scriptlets should be sane
n/a pkgconfig files should go in -devel
+ shouldn't have file dependencies outside /etc /bin /sbin /usr/bin or
/usr/sbin

Notes/Issues:
- I think the package needs to depend on hicolor-icon-theme due to installing icons into dirs owned by that package.
- package contains a desktop file but doesn't validate it. Know isssue with some packages but noting it.

(In reply to comment #8)
> Just two minor notes at the bottom. One being the already acknowledged .desktop
> file issue. Do you know if its been bought up with the packaging committee?

Not sure if packaging committee is the right place to ask. This needs to be fixed in desktop-file-utils upstream. I will ask on the xdg-mailing list, but these things take time. Until then we should just ignore these files.

> - I think the package needs to depend on hicolor-icon-theme due to installing
> icons into dirs owned by that package.

I can add it it you like, but it's not really needed because *all* gtk stuff already requires highcolor-icon-theme.

Ping. Any blockers left?

Looks OK. APPROVED.

Thanks for the review.

New Package CVS Request
=======================
Package Name: xfce4-stopwatch-plugin
Short Description:  Stopwatch plugin for the Xfce panel
Owners: cwickert
Branches: F-10 F-11 F-12
InitialCC:

cvs done.

xfce4-stopwatch-plugin-0.2.0-2.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/xfce4-stopwatch-plugin-0.2.0-2.fc12

xfce4-stopwatch-plugin-0.2.0-2.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/xfce4-stopwatch-plugin-0.2.0-2.fc11

There will be no build for F-10 because the plugin requires Xfce 4-6. Sorry for the confusion.

xfce4-stopwatch-plugin-0.2.0-2.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update xfce4-stopwatch-plugin'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-10533

closing as its in F-12/rawhide

xfce4-stopwatch-plugin-0.2.0-2.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.