Bug 219986 - Review Request: xfce4-smartbookmark-plugin - Smart bookmarks for the Xfce panel
Review Request: xfce4-smartbookmark-plugin - Smart bookmarks for the Xfce panel
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-12-17 21:38 EST by Christoph Wickert
Modified: 2014-09-21 17:51 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-12-22 17:57:30 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Christoph Wickert 2006-12-17 21:38:48 EST
Spec URL: 
http://home.arcor.de/christoph.wickert/fedora/extras/review/SPECS/xfce4-smartbookmark-plugin.spec
SRPM URL: 
http://home.arcor.de/christoph.wickert/fedora/extras/review/SRPMS/xfce4-smartbookmark-plugin-0.4.2-1.fc7.src.rpm
Description: 
A plugin which allows you to do a search directly on Internet on sites like Google or Red Hat Bugzilla. It allows you to send requests directly to your browser and perform custom searches.
Comment 1 Kevin Fenzi 2006-12-22 00:00:05 EST
I'd be happy to review this package. 

Look for a full review in a bit. 
Comment 2 Kevin Fenzi 2006-12-22 00:17:22 EST
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 (GPL)
OK - 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:
284e26595637dd2e900b75534372496b  xfce4-smartbookmark-plugin-0.4.2.tar.gz
284e26595637dd2e900b75534372496b  xfce4-smartbookmark-plugin-0.4.2.tar.gz.1
OK - BuildRequires correct
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.

See below - Package is a GUI app and has a .desktop file

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 - No rpmlint output.
See below - final provides and requires are sane:

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should have dist tag
OK - Should package latest version

Issues:

1. Whats with the commented line in the files section?
#%{_libexecdir}/xfce4/panel-plugins/%{name}

2. Your desktop file needs desktop-file-install:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-254ddf07aae20a23ced8cecc219d8f73926e9755

3. This package provides:
libsmartbookmark.so
Is that correct? Or should that be filtered?
Comment 3 Christoph Wickert 2006-12-22 08:14:46 EST
(In reply to comment #2)
> Issues:
> 
> 1. Whats with the commented line in the files section?
> #%{_libexecdir}/xfce4/panel-plugins/%{name}

This is the new location of xfce4-panel plugins. Hopefully with the next version
 libsmarbookmark.so will be moved from %{_libdir}/xfce4/panel-plugins to
%{_libexecdir}/xfce4/panel-plugins. When all panel plugins are _fully_ ported to
the new panel plugin API you can drop %{_libdir}/xfce4/panel-plugins from you
xfce4-panel package.

> 2. Your desktop file needs desktop-file-install:

Really? I always thought this was only for applications that show up in the main
menu. This desktop file only appears in the "Add Items to the Panel" dialog. I
think in this case there's not need to install the file with
desktop-file-install, because we don't need to modify it (--add-category,
--vendor etc.).

Think of your xfce-utils package, which installs xfce4.desktop in
/usr/share/xsessions or of all the files installed under /etc/xdg/autostart by
xfce4-session and others. None of these packages uses desktop-file-install.

> 3. This package provides:
> libsmartbookmark.so
> Is that correct? Or should that be filtered?

IMO it is correct, but useless.
Comment 4 Patrice Dumas 2006-12-22 09:40:46 EST
(In reply to comment #3)

> > 3. This package provides:
> > libsmartbookmark.so
> > Is that correct? Or should that be filtered?
> 
> IMO it is correct, but useless.

I think that it is incorrect, but harmless. And not worth taking care
of it manually in fedora, it should be handled
* upstream, by not providing the useless soname (which certainly means
  changing libtool)
* at the rpm level, by automatically adding sonames only for sonames
  in %_libdir and directories searched for by ldd
Comment 5 Christoph Wickert 2006-12-22 09:59:02 EST
(In reply to comment #4)

> I think that it is incorrect, but harmless. And not worth taking care
> of it manually in fedora, it should be handled
> * upstream, by not providing the useless soname (which certainly means
>   changing libtool)
> * at the rpm level, by automatically adding sonames only for sonames
>   in %_libdir and directories searched for by ldd

I can certainly talk to upstream about the first, but the second needs to be
fixed in rpm. As we all know rpm is about to change a lot in the near future, so
I don't think that this will be fixed soon.
Comment 6 Kevin Fenzi 2006-12-22 11:51:00 EST
ok, on item 1, perhaps a comment above that line explaining that the plugin will
move there someday?

on item 2, sorry about that. I saw .desktop and didn't look as closely as I
should have. I agree that desktop-file-install is unneeded. 

on item 3, yeah, mentioning it to upstream would probibly be a good idea. 
I wonder if the new rpm.org has a 'wishlist' page we could add that idea to?

I don't see any further issues... so this package is APPROVED. 

Don't forget to close this NEXTRELEASE once it's been imported and built. 
Comment 7 Christoph Wickert 2006-12-22 12:21:35 EST
(In reply to comment #6)
> ok, on item 1, perhaps a comment above that line explaining that the plugin will
> move there someday?

Ok, will do after import.

> I wonder if the new rpm.org has a 'wishlist' page we could add that idea to?

rpm.org is under construction ATM, they don't have a whishlist nor bugzilla.
On http://wiki.rpm.org/ReportingBugs Jesse suggests to file bugs in Fedora's
bugzilla for now.

Thanks for the review, Kevin!
Comment 8 Patrice Dumas 2006-12-22 12:29:24 EST
(In reply to comment #5)

> I can certainly talk to upstream about the first,

I don't think it is worth it, since it is libtool which drives
the link, and (to my knowledge) there is no way to prevent libtool
to add a soname. It is also possible that I am wrong and that a 
soname is needed even if it has no practical use.
Comment 9 Christoph Wickert 2006-12-22 17:57:30 EST
Build on target fedora-development-extras succeeded.
     Build logs may be found at
http://buildsys.fedoraproject.org/logs/fedora-development-extras/24429-xfce4-smartbookmark-plugin-0.4.2-1.fc7/

Closing.
Comment 10 Mukundan Ragavan 2014-09-21 17:45:48 EDT
Package Change Request
======================
Package Name: xfce4-smartbookmark-plugin
New Branches: epel7
Owners: cwickert
InitialCC: nonamedotc
Comment 11 Gwyn Ciesla 2014-09-21 17:51:26 EDT
Git done (by process-git-requests).

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