Bug 819192 - Review Request: xfce4-embed-plugin - Xfce panel plugin to embed various applications
Review Request: xfce4-embed-plugin - Xfce panel plugin to embed various appli...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-05 10:54 EDT by hannes
Modified: 2012-05-17 02:41 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-05-17 02:41:16 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description hannes 2012-05-05 10:54:32 EDT
Spec URL: http://hannes.fedorapeople.org/xfce4-embed-plugin.spec
SRPM URL: http://hannes.fedorapeople.org/xfce4-embed-plugin-1.0.0-0.1.fc17.src.rpm
Description: This plugin enables the embedding of arbitrary
 application windows into the Xfce panel.
The window is resized into the panel space available,
and the associated program can be automatically
launched if it is not open.

rpmlint xfce4-embed-plugin-1.0.0-0.1.fc17.x86_64.rpm 
xfce4-embed-plugin.x86_64: W: spelling-error Summary(en_US) Xfce -> Face
xfce4-embed-plugin.x86_64: W: spelling-error %description -l en_US resized -> resided, re sized, re-sized
xfce4-embed-plugin.x86_64: E: incorrect-fsf-address /usr/share/doc/xfce4-embed-plugin-1.0.0/COPYING
1 packages and 0 specfiles checked; 1 errors, 2 warnings.

I built it against the 4.10 Xfce Version only in a local mock environment with the external repo of kevin. I don't know if it will work with 4.8. Do I need to update this address or is it sufficient to tell upstream about it?
Comment 1 Raphael Groner 2012-05-06 08:20:48 EDT
I am not using fc17. But I can give your spec file a try on PCLinuxOS Phoenix 2012. Looking forward to test the embed plugin with Psi-Plus, a Qt application.
Comment 2 Kevin Fenzi 2012-05-16 13:20:46 EDT
I'll review this. Look for a full review in a bit.
Comment 3 Kevin Fenzi 2012-05-16 13:51:33 EDT
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name. 
OK - Spec has consistant macro usage. 
OK - Meets Packaging Guidelines. 
OK - License (GPLv2+)
See below - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
c5b86cbc1c54ee7c1d2e95a4502fed8e  xfce4-embed-plugin-1.0.0.tar.bz2
c5b86cbc1c54ee7c1d2e95a4502fed8e  xfce4-embed-plugin-1.0.0.tar.bz2.orig
OK - BuildRequires correct
OK - Spec handles locales/find_lang
OK - Package has %defattr and permissions on files is good. 
OK - Package has a correct %clean section. 
OK - Package has correct buildroot
OK - Package is code or permissible content. 
OK - Packages %doc files don't affect runtime. 
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

OK - Package compiles and builds on at least one arch. 
OK - Package has no duplicate files in %files. 
OK - Package doesn't own any directories other packages own. 
OK - Package owns all the directories it creates. 
OK - Package obey's FHS standard (except for 2 exceptions)
See below - No rpmlint output. 
OK - final provides and requires are sane.

SHOULD Items:

OK - Should build in mock. 
OK - Should build on all supported archs
OK - Should function as described. 
OK - Should have sane scriptlets. 
OK - Should have dist tag
OK - Should package latest version
OK - Should not use file requires outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin

Issues: 

1. Why is this release 0.1? It looks like the final 1.0.0?

2. License tag should be "GPLv2+"

3. Your Source0 is: 
Source0:        http://archive.xfce.org/src/panel-plugins/%{name}/%{minor_version}/%{name}-%{version}.tar.bz2

but you don't define %{minor_version} anywhere. ;) 
Add a '%global minor_version 1.0' to the top?

4. rpmlint says: 

Can be ignored: 

xfce4-embed-plugin.i686: W: spelling-error Summary(en_US) Xfce -> Face
xfce4-embed-plugin.i686: W: spelling-error %description -l en_US resized -> resided, re sized, re-sized
xfce4-embed-plugin.src: W: spelling-error Summary(en_US) Xfce -> Face
xfce4-embed-plugin.src: W: spelling-error %description -l en_US resized -> resided, re sized, re-sized

Might ask upstream to update their COPYING file sometime: 

xfce4-embed-plugin.i686: E: incorrect-fsf-address /usr/share/doc/xfce4-embed-plugin-1.0.0/COPYING
xfce4-embed-plugin-debuginfo.i686: E: incorrect-fsf-address /usr/src/debug/xfce4-embed-plugin-1.0.0/panel-plugin/ewmh.h
xfce4-embed-plugin-debuginfo.i686: E: incorrect-fsf-address /usr/src/debug/xfce4-embed-plugin-1.0.0/panel-plugin/ewmh.c
xfce4-embed-plugin-debuginfo.i686: E: incorrect-fsf-address /usr/src/debug/xfce4-embed-plugin-1.0.0/panel-plugin/embed.c
xfce4-embed-plugin-debuginfo.i686: E: incorrect-fsf-address /usr/src/debug/xfce4-embed-plugin-1.0.0/panel-plugin/embed.h
xfce4-embed-plugin-debuginfo.i686: E: incorrect-fsf-address /usr/src/debug/xfce4-embed-plugin-1.0.0/panel-plugin/embed-dialogs.c
xfce4-embed-plugin-debuginfo.i686: E: incorrect-fsf-address /usr/src/debug/xfce4-embed-plugin-1.0.0/panel-plugin/embed-dialogs.h

Should be fixed when minor_version exists: 

xfce4-embed-plugin.src: W: invalid-url Source0: http://archive.xfce.org/src/panel-plugins/xfce4-embed-plugin/%{minor_version}/xfce4-embed-plugin-1.0.0.tar.bz2 HTTP Error 400: Bad Request
3 packages and 0 specfiles checked; 7 errors, 5 warnings.
Comment 4 hannes 2012-05-16 15:52:59 EDT
ad 1) This was a remaining thing, because I initially just did the package for myself.  Fixed
ad 2) Changed, missed it.
ad 3) Fixed it.
ad 4) Upstream is aware of the problem, I think it was/is wrong in their template, they are using for panel-plugins. Talked to the upstream dev and I think he already changed it in master


Updated files:
Spec URL: http://hannes.fedorapeople.org/xfce4-embed-plugin.spec
SRPM URL:
http://hannes.fedorapeople.org/xfce4-embed-plugin-1.0.0-1.fc17.src.rpm
Comment 5 Kevin Fenzi 2012-05-16 16:18:11 EDT
1), 3), 4) all ok. 

I still see: 

License:        GPLv2

in the spec. ;) 

You can change that to GPLv2+ before checking in. 

Provided thats done, I see no further blockers and this package is APPROVED.
Comment 6 hannes 2012-05-16 16:33:41 EDT
Ah yeah, I undo'ed that probably. Will be fixed, sorry.

New Package SCM Request
=======================
Package Name: xfce4-embed-plugin
Short Description: Xfce panel plugin to embed various applications
Owners: hannes
Branches: f17
InitialCC:
Comment 7 Gwyn Ciesla 2012-05-16 22:16:28 EDT
Git done (by process-git-requests).
Comment 8 hannes 2012-05-17 02:41:16 EDT
http://koji.fedoraproject.org/koji/buildinfo?buildID=319305

Build in rawhide

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