Bug 190213

Summary: Review Request: gq - Graphical LDAP directory browser and editor
Product: [Fedora] Fedora Reporter: Terje Røsten <terje.rosten>
Component: Package ReviewAssignee: Christoph Wickert <christoph.wickert>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: 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 Flags
Desktop Patch none

Description Terje Røsten 2006-04-28 20:37:56 UTC
Spec URL: http://www.pvv.ntnu.no/~terjeros/rpms/gq/gq.spec
SRPM URL: http://www.pvv.ntnu.no/~terjeros/rpms/gq/gq-1.0.0-1.src.rpm

Description:

GQ is a graphical browser for LDAP directories and schemas.  Using GQ,
an administrator can search through a directory and modify objects
stored in that directory

Comment 1 Brian Pepple 2006-04-30 20:30:08 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)

Comment 2 Terje Røsten 2006-05-01 20:42:05 UTC
> 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



Comment 3 Brian Pepple 2006-05-01 22:09:13 UTC
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.

Comment 4 Brian Pepple 2006-05-01 22:09:53 UTC
Created attachment 128461 [details]
Desktop Patch

Comment 5 Terje Røsten 2006-05-02 07:07:00 UTC
> 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

Comment 6 Paul Howarth 2006-05-02 11:43:52 UTC
(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.


Comment 7 Terje Røsten 2006-05-02 20:07:57 UTC
> 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


Comment 8 Terje Røsten 2006-05-06 22:25:35 UTC
A-ha, I forgot: I need a sponsor.


Comment 9 Christoph Wickert 2006-05-07 01:08:04 UTC
(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.

Comment 11 Christoph Wickert 2006-11-12 02:18:03 UTC
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'.

Comment 12 Axel Thimm 2006-11-12 13:11:44 UTC
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.

Comment 13 Christoph Wickert 2006-11-12 14:18:39 UTC
(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.

Comment 14 Terje Røsten 2006-11-12 22:00:26 UTC
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



Comment 15 Christoph Wickert 2006-11-19 18:12:12 UTC
(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. 

Comment 16 Terje Røsten 2006-11-19 19:45:57 UTC
> 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


Comment 18 Christoph Wickert 2006-11-20 00:00:54 UTC
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.

Comment 19 Christoph Wickert 2006-11-20 00:20:15 UTC
(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.

Comment 20 Terje Røsten 2006-11-20 15:35:04 UTC
> 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


Comment 21 Christoph Wickert 2006-11-20 17:14:13 UTC
(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.

Comment 22 Terje Røsten 2006-11-20 22:14:11 UTC
> 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.