Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.

Bug 219986

Summary: Review Request: xfce4-smartbookmark-plugin - Smart bookmarks for the Xfce panel
Product: [Fedora] Fedora Reporter: Christoph Wickert <christoph.wickert>
Component: Package ReviewAssignee: Kevin Fenzi <kevin>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: nonamedotc
Target Milestone: ---Flags: gwync: 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: 2006-12-22 22:57:30 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 Christoph Wickert 2006-12-18 02:38:48 UTC
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 05:00:05 UTC
I'd be happy to review this package. 

Look for a full review in a bit. 

Comment 2 Kevin Fenzi 2006-12-22 05:17:22 UTC
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 13:14:46 UTC
(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 14:40:46 UTC
(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 14:59:02 UTC
(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 16:51:00 UTC
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 17:21:35 UTC
(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 17:29:24 UTC
(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 22:57:30 UTC
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 21:45:48 UTC
Package Change Request
======================
Package Name: xfce4-smartbookmark-plugin
New Branches: epel7
Owners: cwickert
InitialCC: nonamedotc

Comment 11 Gwyn Ciesla 2014-09-21 21:51:26 UTC
Git done (by process-git-requests).