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.
I'd be happy to review this package. Look for a full review in a bit.
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?
(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.
(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
(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.
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.
(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!
(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.
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.
Package Change Request ====================== Package Name: xfce4-smartbookmark-plugin New Branches: epel7 Owners: cwickert InitialCC: nonamedotc
Git done (by process-git-requests).