Bug 190139 - Review Request: rapidsvn - Graphical interface for the Subversion version-control system
Review Request: rapidsvn - Graphical interface for the Subversion version-con...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
: 210722 (view as bug list)
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-04-27 18:37 EDT by Tim Jackson
Modified: 2011-11-16 15:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-05-07 04:35:50 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Tim Jackson 2006-04-27 18:37:37 EDT
Spec URL: http://www.timj.co.uk/linux/specs/rapidsvn.spec
SRPM URL: http://www.timj.co.uk/linux/srpms/rapidsvn-0.9.1-1.src.rpm
Description: 
RapidSVN is a GUI front-end for the Subversion revision control system. It
allows access to most of the features of Subversion through a user-friendly
interface.
Comment 1 Jason Tibbitts 2006-04-27 21:55:37 EDT
Oops, it seems I took this package by mistake, but I'll go ahead and do a review.

Right off, there's a build failure on x86_64:
checking for Subversion headers... found
checking for Subversion libraries... not found
configure: error: Subversion libraries are required. Try --with-svn-lib.
error: Bad exit status from /var/tmp/rpm-tmp.54929 (%build)

RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.54929 (%build)
Error building package from rapidsvn-0.9.1-1.src.rpm, See build log
ending

However, the build succeeds on i386.  My assumption is that it isn't checking
/usr/lib64 for the subversion libraries.  You can probably pass
--with-svn-lib="/usr/lib /usr/lib64" to build everywhere.

Issues:
You include the COPYING file, but it contains only the string "Fill me in with
licensing information".  The actual license is more complex than just GPL, it
seems.  LICENSE.txt contains all of the details, so it should probably be
packaged.  I think "GPL and LGPL" would be a more appropriate license tag.

Is autoconf really required?  I didn't see it being called in the build log, and
removing it doesn't seem to hurt anything.

rpmlint complains:
E: rapidsvn library-without-ldconfig-postin /usr/lib/libsvncpp.so.0.0.0
E: rapidsvn library-without-ldconfig-postun /usr/lib/libsvncpp.so.0.0.0

You need to call ldconfig in %post and %postun: see
http://fedoraproject.org/wiki/ScriptletSnippets (the "Shared Libraries" section).

W: rapidsvn devel-file-in-non-devel-package /usr/lib/libsvncpp.so

The guidelines indicate that if you have a versioned .so, the unversioned one
must go in a -devel package.

It seems there's a bunch of headers included in /usr/include; those should
probably go in a -devel package as well.  (Well, they're .hpp files, which I
haven't seen before, but they look like C++ class definitions.)

There does seem to be some kind of test suite (in src/tests/svncpp), but I'm not
sure how you would go about calling it.  This isn't a blocker, but you might
want to take a look.

Review:
* package meets naming and packaging guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
X license field matches the actual license.
X license is open source-compatible and included upstream but is not included in
the package.
* source files match upstream:
   ba03034db35912c7b51b146cc7e6090e  rapidsvn-0.9.1.tar.gz
   ba03034db35912c7b51b146cc7e6090e  rapidsvn-0.9.1.tar.gz-srpm
? BuildRequires are proper (not sure about autoconf)
X package fails to build in mock (development, x86_64).
X rpmlint is silent.
O final provides and requires are sane.
X shared libraries are present; ldconfig should be called but isn't.
* package is not relocatable.
* owns the directory it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* %clean is present.
O %check not present; there seems to be an upstream test suite, but it's not
immediately obvious how it would be called.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
X headers present in main package.
* no pkgconfig files.
* no libtool .la droppings.
* a GUI app; desktop file properly installed with desktop-file-install.
Comment 2 Tim Jackson 2006-04-29 06:42:49 EDT
Thanks very much indeed for your comments Jason. You've raised a number of
interesting points there. I'll take them each in turn.

1. Licensing (inclusion of license documents): fixed, though see also below
about subpackages

2. Build on x86_64; I don't use this architecture but I have added
--with-svn-lib=%_libdir which I think should fix it. Please give it a go. 

3. ldconfig: done

4. autoconf: removed from BR; I don't think it's needed either; it's a leftover
from the spec I originally cannibalised.

5. %check: the tests in src/tests/svncpp are now run

Now onto the bigger issues: looking more closely, rapidsvn bundles (as you
noted) libsvncpp.so*. This is a library which is not strictly related to
rapidsvn, providing as it does an abstract C++ API for Subversion. Now, in an
ideal world we would have svncpp as a totally separate package, but at the
moment it is not distributed as such upstream; only as a bundled part of
rapidsvn. However, it's quite forseeable that that might change (if, for
example, other apps started linking to svncpp then it would quickly become
annoying having it bundled with rapidsvn). So, we should package it reflecting
the fact that it is functionally independent of rapidsvn. Therefore, my updated
spec builds three separate packages:

- svncpp (dynamic, versioned libsvncpp .sos)
- svncpp-devel (headers and unversioned .so)
- rapidsvn (GUI app linked to svncpp)

This also neatly solves the issue of what to put in the License field and what
license docs to include; svncpp and svncpp-devel are LGPL and marked as such and
rapidsvn is GPL.

An open question which I would appreciate advice on (PackageNamingGuidelines has
none) is whether the svncpp package should be called "svncpp" or "libsvncpp".
It's referred to as "svncpp" upstream but many other similar libraries are
packaged named as "libXXX". I don't have any strong feelings either way.

New spec/SRPM:
Spec URL: http://www.timj.co.uk/linux/specs/rapidsvn.spec
SRPM URL: http://www.timj.co.uk/linux/srpms/rapidsvn-0.9.1-2.src.rpm

Tested and builds in mock i386.
rpmlint is now quiet for the SRPM and all three output packages.

There's a bit of an open question about what I should do with LICENSE.txt as
this refers to all three packages (rapidsvn, svncpp, svncpp-devel). I could
include it in all, but even then should it be patched to split off the bits
relevant to each package? Again, advice welcome.

Thanks for your time.
Comment 3 Jason Tibbitts 2006-04-29 16:49:06 EDT
I have no issues with the svncpp name.  I guess if you considered this to be an
addon package for subversion you would call it subversion-cpp, but I'm not sure
it fits into that category.

I don't think you should go editing LICENSE.txt; it should be fine as is as long
as it describes what is being licensed (which it does).

As far as I can tell, the only remaining issue is this new rpmlint warning,
which I've not encountered before so I'll include the full explanation:

E: rapidsvn binary-or-shlib-defines-rpath /usr/bin/rapidsvn ['/usr/lib64']
The binary or shared library defines `RPATH'. Usually this is a
bad thing because it hardcodes the path to search libraries and so
makes it difficult to move libraries around.  Most likely you will find a
Makefile with a line like: gcc test.o -o test -Wl,--rpath.  Also, sometimes
configure scripts provide a --disable-rpath flag to avoid this.

No --disable-rpath in configure, but lots of one libtool and two g++ invocations
that reference it.  Unfortunately I'm not sure how to get rid of them; autoconf
is completely opaque and hideous to me.
Comment 4 Jason Tibbitts 2006-04-29 17:50:16 EDT
Every recommendation outside of patching configure given on
http://fedoraproject.org/wiki/Extras/Schedule/RpathCheckBuildsys with no luck. 
export LIBTOOL=libtool didn't work; calling libtoolize --force didn't work (but
maybe that wouldn't be enough to make it work).

I even deleted every instance of -rpath from configure and aclocal.m4 and while
g++ is never called with it, it still shows up in the final executable.  I give up.
Comment 5 Tim Jackson 2006-04-29 18:17:01 EDT
Hmm, this doesn't seem to show up on x86; rpmlint is quiet. I also know nothing
about this issue other than what I've read on the page you cited, so I can't
help unfortunately :(

The page you cited calls it a "minor" issue (for standard RPATHs) so I presume
it doesn't necessarily stand in the way of inclusion.

Perhaps one to throw at extras-list and see if anyone else can help?
Comment 6 Jason Tibbitts 2006-04-29 21:52:24 EDT
edhill on IRC pointed out that I had missed an -rpath call in
src/svncpp/Makefile.in; removing it breaks the build in a rather odd way; the
library is build but it seems some libtool thing doesn't get generated
(.libs/libsvncpp.lai) and things die.

So I really am out of options.  I'll ask extras-list if this can be ignored.
Comment 7 Jason Tibbitts 2006-05-01 05:58:04 EDT
Based on input from extras-list, if you append LIBTOOL=/usr/bin/libtool to the
make line, and then in %install delete the extra libsvncpp.a that crops up,
everything comes out OK (FC5 and development, i386 and x86_64).
Comment 8 Tim Jackson 2006-05-04 07:05:49 EDT
OK, I've added the libtool fix. Thanks very much for all your investigation work
on this unusual problem, Jason. Can you give the following a go?

New spec/SRPM:
Spec URL: http://www.timj.co.uk/linux/specs/rapidsvn.spec
SRPM URL: http://www.timj.co.uk/linux/srpms/rapidsvn-0.9.1-3.src.rpm
Comment 9 Jason Tibbitts 2006-05-04 16:27:34 EDT
OK, with the libtool fix everything is fixed.  Rawhide is broken at the moment
so no development x86_64 builds (keep that in mind when you check in).

So, to recap the initial issues:

X license field matches the actual license.
Fixed.

X license is open source-compatible and included upstream but is not included in
the package.
Fixed.

X package fails to build in mock (development, x86_64).
Fixed (bulds properly; not your fault rawhide is busted)

X rpmlint is silent.
It's been quieted.

X shared libraries are present; ldconfig should be called but isn't.
ldconfig is called properly.

X headers present in main package.
Headers have been moved off to a -devel subpackage.

I have no remaining objections.

APPROVED
Comment 10 Tim Jackson 2006-05-07 04:35:50 EDT
Built successfully for FC-5 and devel (jobs #8854 and #8949)
Comment 11 Mamoru TASAKA 2006-10-13 19:22:44 EDT
*** Bug 210722 has been marked as a duplicate of this bug. ***
Comment 12 Tim Jackson 2011-11-16 14:24:56 EST
Package Change Request
======================
Package Name: rapidsvn
New Branches: el6
Comment 13 Jon Ciesla 2011-11-16 14:57:09 EST
No owners listed.
Comment 14 Tim Jackson 2011-11-16 15:07:49 EST
Package Change Request
======================
Package Name: rapidsvn
New Branches: el6
Owners: timj
Comment 15 Jon Ciesla 2011-11-16 15:11:59 EST
Git done (by process-git-requests).

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