Bug 190213
Summary: | Review Request: gq - Graphical LDAP directory browser and editor | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Terje Røsten <terje.rosten> | ||||
Component: | Package Review | Assignee: | Christoph Wickert <christoph.wickert> | ||||
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | medium | ||||||
Version: | rawhide | CC: | axel.thimm, cweyl | ||||
Target Milestone: | --- | Keywords: | Reopened | ||||
Target Release: | --- | ||||||
Hardware: | All | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2006-11-24 19:25: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 | ||||||
Attachments: |
|
Description
Terje Røsten
2006-04-28 20:37:56 UTC
Couple of quick notes: 1. The handling of the desktop file is incorrect. Refer to http://fedoraproject.org/wiki/Packaging/Guidelines#head-254ddf07aae20a23ced8cecc219d8f73926e9755 2. Unnecessary BuildRequires: krb5-devel (provided by openssl-devel) > 1. The handling of the desktop file is incorrect. Refer to > http://fedoraproject.org/wiki/Packaging/Guidelines#head-254ddf07aae20a23ced8cecc219d8f73926e9755 Thanks for the ref, fixed. > 2. Unnecessary BuildRequires: krb5-devel (provided by openssl-devel) OK, fixed. New, improved package available here: Spec: http://www.pvv.ntnu.no/~terjeros/rpms/gq/gq.spec SRPM: http://www.pvv.ntnu.no/~terjeros/rpms/gq/gq-1.0.0-2.src.rpm It would be a little easier to patch the current desktop file included in the tarball, instead of adding an additional one. That way you wouldn't be having to exclude (which really should be removed instead) the old desktop file. Your desktop-file-install call would be like this: desktop-file-install --vendor fedora --delete-original \ --dir %{buildroot}%{_datadir}/applications \ --add-category X-Fedora \ %{buildroot}%{_datadir}/applications/%{name}.desktop I'll attach the patch for you to this bug. Created attachment 128461 [details]
Desktop Patch
> It would be a little easier to patch the current desktop file Done. -3 ready: Spec: http://www.pvv.ntnu.no/~terjeros/rpms/gq/gq.spec SRPM: http://www.pvv.ntnu.no/~terjeros/rpms/gq/gq-1.0.0-3.src.rpm (In reply to comment #5) > > It would be a little easier to patch the current desktop file > > Done. -3 ready: > > Spec: http://www.pvv.ntnu.no/~terjeros/rpms/gq/gq.spec > SRPM: http://www.pvv.ntnu.no/~terjeros/rpms/gq/gq-1.0.0-3.src.rpm Hmm, it seems that gq is back from the dead. I actually came to look at this review request with a view to seeing if you'd looked at lat (Bug #177580) as an alternative to gq, since gq was dead when I was looking for a GUI LDAP tool myself last year. I'll have to go and look at gq myself now ;-) Anyway, one quick comment on the spec: rather than using a specific mirror in the Source URLs, try using URLs like this: Source0: http://dl.sf.net/gqclient/gq-%{version}.tar.gz Not only will that potentially use any mirror, it's a bit shorter too. > Anyway, one quick comment on the spec: rather than using a specific mirror in > the Source URLs, try using URLs like this: > > Source0: http://dl.sf.net/gqclient/gq-%{version}.tar.gz Sure, fixed. We are at -4: Spec: http://www.pvv.ntnu.no/~terjeros/rpms/gq/gq.spec SRPM: http://www.pvv.ntnu.no/~terjeros/rpms/gq/gq-1.0.0-4.src.rpm A-ha, I forgot: I need a sponsor. (In reply to comment #8) > A-ha, I forgot: I need a sponsor. Then you should note close this bug but at it do the FE-NEEDSPONSOR tracker bug ;) Done and reopened. Updated to 1.0.1: Spec: http://www.pvv.ntnu.no/~terjeros/rpms/gq/gq.spec SRPM: http://www.pvv.ntnu.no/~terjeros/rpms/gq/gq-1.0.1-1.fc5.src.rpm Terje, are you still interested in this package? If so, please update the package to the latest stable version (1.2.1 I think), I will review it then. BTW: Please use 'make install DESTDIR=$RPM_BUILD_ROOT' instead if '%makeinstall'. If Terje isn't interested anymore I would like to pick it up, since I have gq in ATrpms (I'd have to check the procedure in the wiki first). Terje, if you're still on it, please check the specfile at ATrpms for any possible merged specfile, thanks! BTW the gq homepage seems to be down currently, so an updated specfile (and even a review of the current tarball md5sums) is not easy to do. (In reply to comment #12) > BTW the gq homepage seems to be down currently, so an updated specfile (and even > a review of the current tarball md5sums) is not easy to do. The files _should_ be available at http://dl.sf.net/sourceforge/gqclient/gq-1.2.1.tar.gz and http://dl.sf.net/sourceforge/gqclient/gq-1.2.1-langpack-1.tar.gz but I could not reach dl.sf.net ether and had to use a mirror. So, yes, we have no valid URL for the specfile ATM. Updated to gq-1.2.1, I did this some time ago, but something is this version broke the install of po files. I was waiting for upstream do a new release. Anyway, new version with some improvements is available here: SPEC: http://www.pvv.ntnu.no/~terjeros/rpms/gq/gq.spec SRPMS: http://www.pvv.ntnu.no/~terjeros/rpms/gq/gq-1.2.1-2.fc6.src.rpm (In reply to comment #14) > Updated to gq-1.2.1, I did this some time ago, but something is this version > broke the install of po files. I was waiting for upstream do a new release. You need to copy LINGUAS from gq-%{version}-langpack-1/po to the main packages po dir before compiling, e. g. like this: # Set up langpack %{name}-%{version}-langpack-1/langpack . %{__cp} -p %{name}-%{version}-langpack-1/po/LINGUAS po/ ... Then re-add %find_lang and %files -f %{name}.lang to the spec. Some more things I'd like to you to fix, before I do the complete review: - Please BuildRequire "perl(XML::Parser)" instead of perl-XML-Parser (better user the name of the perl module than the hardcoded package name) - duplicate BuildRequires: gtk2-devel and libxml2-devel are already required by libglade2-devel. - no need to run /sbin/ldconfig in %post, we don't have no shared libs here - please use the scriptlets from the wiki for update-mime-database, see http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-5f93ed83c968bb73b052c06ba0d7139e28f35d93 If you are "stealing" something (your own words from changelog), why not steal from the wiki? ;) - The icon is missing, at least here on Core 6. "redhat-system-tools" is outdated, now it's "redhat-system_tools" (note the underscore). - Don't add the category X-Fedora to gq.desktop, it's not necessary any longer. In fact it never was. - Minor: Can you make your desktop file look more like the upstream one? I'd like to see the multilingual names and comments included. - Minor: you could simplify the %files section a little more, e.g. you could replace %{_datadir}/%{name}/%{name}.glade and %dir %{_datadir}/%{name} with a single "%{_datadir}/%{name}/". This is ok as long as all files in the dir should be owned by gq, which is the case for %{_datadir}/%{name} and %{_datadir}/pixmaps/%{name}. If you however prefer a more detailed listing this is ok too, it will prevent you from accidentally packaging unwanted files. - Minor: Although your defattr is valid I suggest you use "%defattr(-,root,root,-)" instead. This is FE default and looks cleaner. You can also remove the superfluous macros: "%{__make}" "%{__install}" or "%{__rm}" or don't have a practical benefit over make, install or rm. - Minor: Please fix the last changelog entry: Nov 12th was Sun, not Sat. > Some more things I'd like to you to fix, before I do the complete review: Most things should be fixed now, from the changelog: - Fix defattr - Remove X-Fedora as category in desktop file - libglade2-devel pulls in gtk2-devel and libxml2-devel in BuildReq - Use perl modules, not perl package name in BuildReq - Remove ldconfig from %%post - Fix install of translations - Remove some macros (rm, make and install) - Drop desktop-file-utils from BuildReq - Fix mime: script and remove shared-mime-info from BuildReq - Patch, not replace desktop file - Switch icon to redhat-system_tools Thanks for the initial review and the LINGUAS trick. Updates files: SPEC: http://www.pvv.ntnu.no/~terjeros/rpms/gq/gq.spec SRPMS: http://www.pvv.ntnu.no/~terjeros/rpms/gq/gq-1.2.1-3.fc6.src.rpm Updated to 1.2.2 SPEC: http://www.pvv.ntnu.no/~terjeros/rpms/gq/gq.spec SRPMS: http://www.pvv.ntnu.no/~terjeros/rpms/gq/gq-1.2.2-1.fc6.src.rpm Thanks, you are pretty fast with your update. You will have to update your package if there will be a language pacak for 1.2.2. Anyway, here we go: REVIEW for c1b303242245dd6fc2c04bb1a9d020b6 gq-1.2.2-1.fc6.src.rpm FAIL - rpmlint isn't happy with your srpm: $ rpmlint gq-1.2.2-1.fc6.src.rpm W: gq patch-not-applied Patch01: gq-1.2.1-desktop.patch This is because the patch is defined as Patch01:, but it's called with %patch1. You should change it to Patch0: anyway, since it's the first patch. Same for Source: and Source01:, should be Source0: and Source1: instead. rpmlint on the binaries is clean. OK - package meets naming guidelines OK - specfile named correctly OK - package meets packaging guidelines OK - license open-source compatible (GPL) OK - license included in source OK - license field in specfile machtes actual license OK - license included in %doc OK - specfile in American English OK - specfile is legible OK - source matches upstream OK - package builds for Core 6 on i386 OK - BuildRequires are correct, not exeptions, no duplicates OK - locales are handled correctly with %find_lang.sh OK - package is not relocatable OK - package owns all directories that it creates OK - no duplicate files in the %files listing OK - permissions are set properly, correct %defattr: OK - %clean section present OK - macro usage consistent (using macro style) OK - code, not content OK - no large docs OK - docs don't affect the program's runtime OK - no headers static libs or pc files OK - no need for a devel package OK - no libtool archives OK - package correctly uses desktop-file-install OK - package doesn't own dirs already owned by other packages OK - package works as described OK - package uses scriptlets from the wiki FAIL - package doesn't build in mock, an error occurs while building the locales: > Making all in po > make[2]: Entering directory `/builddir/build/BUILD/gq-1.2.2/po' > file=`echo cs | sed 's,.*/,,'`.gmo \ > && rm -f $file && -o $file cs.po > /bin/sh: line 1: -o: command not found > make[2]: *** [cs.gmo] Error 127 > make[2]: Leaving directory `/builddir/build/BUILD/gq-1.2.2/po' > make[1]: *** [all-recursive] Error 1 > make[1]: Leaving directory `/builddir/build/BUILD/gq-1.2.2' > make: *** [all] Error 2 > error: Bad exit status from /var/tmp/rpm-tmp.23553 (%build) the 4th line should be > && rm -f $file && /usr/bin/msgfmt -o $file cs.po You need to BuildRequire gettext to provide msgfmt. FIX - Remove the line about desktop-file-utils from changelog entry of 1.2.1-3. The line is wrong, desktop-file-utils is still included in your spec and this is correct. Simply drop that line. FIX - Although the package builds in mock you should add a buildrequirement on libgcrypt-devel (configure checks for /usr/bin/libgcrypt-config) FIX? - You should also consider libgpg-error-devel, it's used if available too. I have to admit don't know if it adds any value to the package, but IMO it can't hurt. MINOR - After you have removed useless macros you can also replace "%{__cp} -p" with a simple "cp" (when copying LINGUAS) MINOR - IMHO %post and %postun should not be after the %files section MINOR - You can make %post and %post un a little smarter > %post -p update-mime-database %{_datadir}/mime &> /dev/null || : > > %postun -p update-mime-database %{_datadir}/mime &> /dev/null || : This has the advantage the rpm will automagically care for the requirement on update-mime-database/shared-mime-info Please fix the blockers (FAIL and FIX), so that I can approve the package. (In reply to comment #18) > > FIX? - You should also consider libgpg-error-devel, it's used if available too. Oops, I just realized that it's already required by libgcrypt-devel, so that would be a duplicate BR. > You should change it to Patch0: anyway, since it's the first patch. Same for > Source: and Source01:, should be Source0: and Source1: instead. Fixed. > FAIL - package doesn't build in mock, an error occurs while building the locales: > You need to BuildRequire gettext to provide msgfmt. Fixed. > FIX - Remove the line about desktop-file-utils from changelog entry of 1.2.1-3. > The line is wrong, desktop-file-utils is still included in your spec and this is > correct. Simply drop that line. Fixed. > FIX - Although the package builds in mock you should add a buildrequirement on > libgcrypt-devel (configure checks for /usr/bin/libgcrypt-config) Fixed. > MINOR - After you have removed useless macros you can also replace "%{__cp} -p" > with a simple "cp" (when copying LINGUAS) Fixed. > MINOR - IMHO %post and %postun should not be after the %files section Fixed. > MINOR - You can make %post and %post un a little smarter > > > %post -p update-mime-database %{_datadir}/mime &> /dev/null || : > > > > %postun -p update-mime-database %{_datadir}/mime &> /dev/null || : > > This has the advantage the rpm will automagically care for the requirement on > update-mime-database/shared-mime-info I am able to don't get this to work, should it? New files: SPEC: http://www.pvv.org/~terjeros/rpms/gq/gq.spec SRPMS: http://www.pvv.org/~terjeros/rpms/gq/gq-1.2.2-2.fc6.src.rpm (In reply to comment #20) > > update-mime-database/shared-mime-info > > I am able to don't get this to work, should it? erm, no sorry, this was too much for rpm. Only takes one agrument and needs an absolute path. Doesn't really matter. > New files: > SPEC: http://www.pvv.org/~terjeros/rpms/gq/gq.spec > SRPMS: http://www.pvv.org/~terjeros/rpms/gq/gq-1.2.2-2.fc6.src.rpm The new package 998cd6dc0290925aef60d1786f96978f gq-1.2.2-2.fc6.src.rpm - builds in mock - is fine for rpmlint - and fixes all issuse mentioned in comment #18 This package is APPROVED One last thing I'd like to mention: I would remove the lines about desktop-file-utils from the changelog without further notice. They are confusing people. Although a detailed changelog in general is a good thing, your changelogs tend to be a little to elaborate. Some changes could easily be summarized as "clean up specfile" or something like that. I see you already are sponsored by Ed Hill, so I remove the Bug #177841 Tracker. Do you have any other pending reviews or need further help? Please keep in mind that FE CVS is currently broken, so you can't import your package. > This package is APPROVED Thanks. > Do you have any other pending reviews or need further help? Can you please take a look at #216519 (just submitted). > Please keep in mind that FE CVS is currently broken, so you can't import your > package. Yeah, so I understand. |