Bug 226189

About to get kicked out of the reviewing room, but I'll at least list out the
rpmlint issues:

W: neon-devel summary-ended-with-dot Development libraries and C header files
for the neon library.
  Should be trivial to fix.

W: neon redundant-prefix-tag
  Prefix: should just be deleted.  Relocatable packages are a lost cause anyway.

Also, current upstream neon is 0.26.3.  I think there are API changes that may
prevent this from being a simple update, so I'll let you make that decision. 
0.25.5 is the most current 0.25.5 release available.

I've fixed the rpmlint issues in -6; thanks.

Interestingly, once the package is installed, rpmlint produces the following
additional warnings:

W: neon unused-direct-shlib-dependency /usr/lib64/libneon.so.25.0.5
/usr/lib64/libkrb5.so.3
W: neon unused-direct-shlib-dependency /usr/lib64/libneon.so.25.0.5
/usr/lib64/libk5crypto.so.3
W: neon unused-direct-shlib-dependency /usr/lib64/libneon.so.25.0.5
/lib64/libcom_err.so.2
W: neon unused-direct-shlib-dependency /usr/lib64/libneon.so.25.0.5
/lib64/libresolv.so.2
W: neon unused-direct-shlib-dependency /usr/lib64/libneon.so.25.0.5
/lib64/libdl.so.2

I will admit to not having a clue as to what might cause this or how to get rid
of it, as I've never seen this warning from rpmlint before.

I note there are a few Conflicts with an extremely old version of subversion. 
Even the subversion in FC1 was newer than the problem version, so there's really
no reason for the Conflicts bits to be there these days.

There seems to be a test suite and according to %changelog it was run at some
point but doesn't seem to be run now.  I added a %check section and got things
to run until:

./uri-tests: error while loading shared libraries: libz.so.1: failed to map
segment from shared object: Cannot allocate memory

Not sure what's up there, so probably best not to try to run the test suite.

The -devel package includes both a static library and a .la file.  According to
the guidelines, neither of these should be there except in exceptional
circumstances.

Review:
* source files match upstream:
   b5513f88cb54c5f11e4c8348ee6c7ace9767b45c263c3a3ba8a5ce4e2b40a07a
   neon-0.25.5.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
O dist tag is not present (not required)
* build root is correct.
* license field matches the actual license.
* license is open source-compatible (LGPL)
* License text included in package.
* latest version is being packaged (latest in the 0.25 series)
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
* debuginfo package looks complete.
? rpmlint has complaints, but I'm not sure what they mean.
* final provides and requires are sane:
  neon-0.25.5-6.x86_64.rpm
   libneon.so.25()(64bit)
   neon = 0.25.5-6
  =
   /sbin/ldconfig
   libcom_err.so.2()(64bit)
   libcrypto.so.6()(64bit)
   libexpat.so.0()(64bit)
   libgssapi_krb5.so.2()(64bit)
   libgssapi_krb5.so.2(gssapi_krb5_2_MIT)(64bit)
   libk5crypto.so.3()(64bit)
   libkrb5.so.3()(64bit)
   libneon.so.25()(64bit)
   libssl.so.6()(64bit)
   libz.so.1()(64bit)

  neon-devel-0.25.5-6.x86_64.rpm
   neon-devel = 0.25.5-6
  =
   /bin/sh
   expat-devel
   libneon.so.25()(64bit)
   neon = 0.25.5-6
   openssl-devel
   pkgconfig
   zlib-devel

* %check is not present; the included test suite doesn't actually run.
* shared libraries present; ldconfig is called properly.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* scriptlets are OK (ldconfig)
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headers are in the -devel subpackage.
* a pkgconfig file is present and in the -devel package; pkgconfig is a
  dependency.
X a .la file is present, as well as a static library.

Thanks for the review.

- the test suite will run but reserves a fixed port so cannot be run reliably
during build (multiple simultaneous builds on a single host will not necessarily
work); (for x86_64 it fails in 0.25 due to a restrictive ulimit -v; 'sed -i
/ulimit/d' test/run.sh' will fix that)

- the .la file is part of the defined interface so will not be dropped.  (it's
used by third-party apps via "neon-config --la-file"

- the static archive is used by the static build of RPM (which links against
neon), so is required

- the rpmlint unused-direct-dep warnings can't be easily fixed (properly, anyway)

OK, that all seems reasonable.  The current policy is supposed to be to require
static libs to be in a -static subpackage, but for some reason that isn't
actually in the guidelines although it was ratified in November.  So I certainly
won't block on that.

It would be nice to get at least a couple of comments about why the .la and .a
files are needed, and about why the test suite can't be run, into the .spec so
that the next person who takes a look at it will at least know why they're there.

APPROVED

If the fixed package has been built for rawhide, you can go ahead and close this
bug.

Done now - thanks for your time!

*.pc patch forthcoming to move static lib deps to Libs.private (similar to
recent patches for apr/apr-util)

Created attachment 151989 [details]
neon.pc: move static-only libs to Libs.private section

Here's a few more items that should be addressed in merge review:

1. neon's static libs (and .la's) file should be packaged separately:
into a -static or -devel-static package:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-82d97fc4a3421310f4e2971180e4165965b65662
http://fedoraproject.org/wiki/PackagingDrafts/StaticLinkage

This will be mildly painful, but to do this right, we'll have to sort through
the whole stack of dependent libs/packages and do those too.

2. libneon.la (currently in neon-devel) contains references to -lgssapi_krb5
-lkrb5 but it doesn't include:
the (sub) package owning libneon.la MUST include:
Requires: krb5-devel
Eventually, when/if krb5-static exists, neon-static needs
Requires: krb5-static

Just a note that the static library guideline wasn't there when I originally
reviewed this package, so this bit about splitting them (and the .la file) out
from the main package is new.

Still, I'm not sure what the value of putting the .la file in a separate package
is if the main package will just require it in order for "neon-config --la-file"
to be meaningful.  Unless, of course, it's OK for neon-config output to point to
a nonexistant file in the case that the -static package is not installed.

Re: neon-config --la-file

Prolly patch
--- neon-config.static  2006-07-12 12:13:28.000000000 -0500
+++ neon-config 2007-04-13 10:27:56.000000000 -0500
@@ -82,7 +82,9 @@
        ;;

     --la-file)
+       if test -f ${libdir}/libneon.la ; then
        echo ${libdir}/libneon.la
+       fi
        ;;

     --support)

Rex: thanks for the .pc patch, looks great (neon 0.26.x does similar already).

But breaking the --la-file output is simply not acceptable.

I'm not sure it is worth the hassle of going through creating all those -static
packages.  We could just drop the libneon.a, strip the dep_libs line from the
.la file to prevent deps leaks, break the upstream RPM build, and be done.

> break the upstream RPM build, and be done.

you sure?  Having the ability to have a static rpm seems desirable (but you're
right, the extra effort may not be worth the pain).

> breaking the --la-file output is simply not acceptable.

why?  really, who uses it?  what depends on it?  couldn't/shouldn't those be fixed?

Joe, could you please provide a more comprehensive justification for .la files
than the terse "not acceptable"?

It's part of the neon-config interface as defined upstream, and breaking
interfaces is unacceptable.

If that's all you've got, then I still don't buy it (sorry).  
What I'd *really* like to see is concrete examples of breakage.

What do you not buy?  That it's part of the interface as defined upstream, or
that breaking interfaces is unacceptable?

I buy neither (yet).

Of course omitting .la files sometimes involves a little collateral damage.  The
short-term pain/damage in *most* cases outweighes the pain/suffering caused by
.la file presence in rpm packaging (ie, why Fedora packaging policy to omit .la
files by default exists).  Now, without knowing any details of the "collateral
damage" to which you infer, it's not possible to judge whether it merits an
exception.

The interface in question is `neon-config --la-file`, which is documented in
"man neon-config", and also by `neon-config --help`.  Can you explain why you
don't think this is an interface defined upstream?

I don't think the advantage of keeping the upstream interface outweighs the
packaging beneifits of omitting it.

Again, do you have any examples of concrete breakage in other packages/software
that depend on this interface, or not?  I'd like to explore the full
ramifications of removing it.

Joe, I don't think Rex's request for examples of something that would actually
break if the .la files were removed is unreasonable.  Could you please provide a
few?

The guidelines are there for a reason and having that extra information may make
a case either way for providing an exception or not.

It's not actually relevant whether or not I can give examples of apps which
break if the .la file is removed.  The point is that doing so breaks an
interface which is defined upstream (`neon-config --la-file`).  It is a golden
rule of packaging that you do not break interfaces defined upstream.

But, examples I know of: Subversion, which uses --la-file to link against neon,
and I think also the static build of RPM (at least upstream), which IIRC on
libtool resolving dependencies via the .la file -- exactly what .la files are
designed for.

Let me (re)iterate that the "golden rule of packaging" is trumped if:
1.  following it violates packaging guidelines
and/or
2.  upstream interface is stooopid.

re: static linking.  This is a valid use-case for keeping .la files, but these
need to be packaged separately in a -static (sub)package.  But you had already
stated (in comment #13) "I'm not sure it is worth the hassle of going through
creating all those -static packages.  We could just drop the libneon.a ...", in
which case, this is moot.

In non-static cases, packages that depend on .la presence for building, are,
imo, broken, and can/should be fixed.  I'll take the bait and look at subversion
and see how hard it is to fix.

Playing the "We're smarter than upstream" game is a losing proposition in the
long term, and one I will not indulge in for any package I maintain.  Fedora
does not exist in a vacuum, it exists in main due to the work done in upstream
projects.  If you have changes which you think can improve the software you
should be getting involved in the upstream projects to get those changes
integrated.  This may help you better understand the motivation behind things
which you consider to be "stupid".

When/if the .la dependence is fixed, the changes most certainly should be pushed
upstream.  Your assertion that this be a prerequisite to any modification of
fedora packaging is misplaced, imo.

And I stand by my statement, any app that needs help to build via 'neon-config
--la-file' and fails without libneon.la presence, is broken by design (hopefully
that's nicer than saying stooopid).

I'm making progress wrt subversion... looks like a one-line patch is all that is
required(1):
--- subversion-1.4.3/build/ac-macros/neon.m4.la_file    2006-10-20
18:44:09.000000000 -0500
+++ subversion-1.4.3/build/ac-macros/neon.m4    2007-04-27 11:34:44.000000000 -0500
@@ -141,7 +141,7 @@
            test "$svn_allowed_neon" = "any"; then
             svn_allowed_neon_on_system="yes"
             SVN_NEON_INCLUDES=[`$neon_config --cflags | sed -e 's/-D[^ ]*//g'`]
-            NEON_LIBS=`$neon_config --la-file`
+            NEON_LIBS=`$neon_config --libs`
             CFLAGS=["$CFLAGS `$neon_config --cflags | sed -e 's/-I[^ ]*//g'`"]
             svn_lib_neon="yes"
             break


any others?


(1) Though one could *greatly* simplify it's content/logic by simply using
pkg-config constructs, esp since pkg-config is able to differentiate between
static and non-static configs (but that goes beyond the scope of this discussion)

w.r.t. the original review: the static libneon.a has been dropped in Raw Hide,
since we haven't linked rpm linked against neon for a while.  The dep_libs are
now also stripped from the .la file to prevent dep leaks.

The goal of removing .la files is to prevent unnecessary dep leaks; that goal
has now been achieved, without removing the .la file and breaking the
neon-config interface.

I'll have to remove myself, at least for now (-ENOTIME), to open the door for
someone else to help here.

Joe, thanks for your efforts re: .la files, that's likely a reasonable compromise.

neon.i386: W: file-not-utf8 /usr/share/doc/neon-0.28.3/THANKS
neon.i386: W: no-version-in-last-changelog

I've fixed the former upstream; not sure what is causing the latter, the last changelog entry looks fine to me

* Thu Aug 28 2008 Joe Orton <jorton> 0.28.3-2
- update to 0.28.3

Okay, then feel free to ignore it. If find time, I'll go for a full review in
the next time; if there are takers before, feel free as well.

[ DONE ] MUST: rpmlint must be run on every package. The output should be 
         posted in the review
         $ rpmlint /var/lib/mock/fedora-10-x86_64/result/neon-*
         neon.x86_64: W: file-not-utf8 /usr/share/doc/neon-0.28.3/THANKS
         4 packages and 0 specfiles checked; 0 errors, 1 warnings.
         $
         -> Solved upstream as per comment #30 and will pop into Fedora with 
            the next release at latest, that's okay for me and has to be okay
            for Fedora as well therefore ;-)
[  OK  ] MUST: The package must be named according to the Package Naming 
         Guidelines
[  OK  ] MUST: The spec file name must match the base package %{name}, in the 
         format %{name}.spec unless your package has an exemption 
[  OK  ] MUST: The package must meet the Packaging Guidelines
[  OK  ] MUST: The package must be licensed with a Fedora approved license and 
         meet the Licensing Guidelines
[  OK  ] MUST: The License field in the package spec file must match the 
         actual license.
[  OK  ] MUST: If (and only if) the source package includes the text of the 
         license(s) in its own file, then that file, containing the text of 
         the license(s) for the package must be included in %doc
[  OK  ] MUST: The spec file must be written in American English
[  OK  ] MUST: The spec file for the package MUST be legible.
[  OK  ] MUST: The sources used to build the package must match the upstream 
         source, as provided in the spec URL. Reviewers should use md5sum for 
         this task. If no upstream URL can be specified for this package, 
         please see the Source URL Guidelines for how to deal with this.
         -> 47599a328862ce64ac3c52726d6daa12  neon-0.28.3.tar.gz
         -> 47599a328862ce64ac3c52726d6daa12  neon-0.28.3.tar.gz.1
         -> 544a92dbfba144ec600506cadbda92ae0b0eb9b0  neon-0.28.3.tar.gz
         -> 544a92dbfba144ec600506cadbda92ae0b0eb9b0  neon-0.28.3.tar.gz.1
[  OK  ] MUST: The package MUST successfully compile and build into binary 
         rpms on at least one primary architecture.
[  N/A ] MUST: If the package does not successfully compile, build or work on 
         an architecture, then those architectures should be listed in the 
         spec in ExcludeArch. Each architecture listed in ExcludeArch MUST 
         have a bug filed in bugzilla, describing the reason that the package 
         does not compile/build/work on that architecture. The bug number MUST 
         be placed in a comment, next to the corresponding ExcludeArch line. 
[  OK  ] MUST: All build dependencies must be listed in BuildRequires, except 
         for any that are listed in the exceptions section of the Packaging 
         Guidelines ; inclusion of those as BuildRequires is optional. Apply 
         common sense.
[  N/A ] MUST: The spec file MUST handle locales properly. This is done by 
         using the %find_lang macro. Using %{_datadir}/locale/* is strictly 
         forbidden.
[  OK  ] MUST: Every binary RPM package (or subpackage) which stores shared 
         library files (not just symlinks) in any of the dynamic linker's 
         default paths, must call ldconfig in %post and %postun.
[  N/A ] MUST: If the package is designed to be relocatable, the packager must 
         state this fact in the request for review, along with the 
         rationalization for relocation of that specific package. Without 
         this, use of Prefix: /usr is considered a blocker.
[  OK  ] MUST: A package must own all directories that it creates. If it does 
         not create a directory that it uses, then it should require a package 
         which does create that directory.
[  OK  ] MUST: A package must not contain any duplicate files in the %files 
         listing.
[  OK  ] MUST: Permissions on files must be set properly. Executables should 
         be set with executable permissions, for example. Every %files section 
         must include a %defattr(...) line.
[  OK  ] MUST: Each package must have a %clean section, which contains rm -rf
         %{buildroot} (or $RPM_BUILD_ROOT).
[  OK  ] MUST: Each package must consistently use macros.
[  OK  ] MUST: The package must contain code, or permissable content.
[  N/A ] MUST: Large documentation files must go in a -doc subpackage. (The 
         definition of large is left up to the packager's best judgement, but 
         is not restricted to size. Large can refer to either size or 
         quantity).
[  OK  ] MUST: If a package includes something as %doc, it must not affect the 
         runtime of the application. To summarize: If it is in %doc, the 
         program must run properly if it is not present
[  OK  ] MUST: Header files must be in a -devel package
[  N/A ] MUST: Static libraries must be in a -static package
[  OK  ] MUST: Packages containing pkgconfig(.pc) files must 'Requires: 
         pkgconfig' (for directory ownership and usability).
[  OK  ] MUST: If a package contains library files with a suffix (e.g. 
         libfoo.so.1.1), then library files that end in .so (without suffix) 
         must go in a -devel package.
[  OK  ] MUST: In the vast majority of cases, devel packages must require the 
         base package using a fully versioned dependency: Requires: %{name} =
         %{version}-%{release}
[  OK  ] MUST: Packages must NOT contain any .la libtool archives, these must 
         be removed in the spec if they are built
         -> Special case, already discussed above and thereby accepted.
[  N/A ] MUST: Packages containing GUI applications must include a 
         %{name}.desktop file, and that file must be properly installed with 
         desktop-file-install in the %install section. If you feel that your 
         packaged GUI application does not need a .desktop file, you must put 
         a comment in the spec file with your explanation.
[  OK  ] MUST: Packages must not own files or directories already owned by 
         other packages. The rule of thumb here is that the first package to 
         be installed should own the files or directories that other packages 
         may rely upon. This means, for example, that no package in Fedora 
         should ever share ownership with any of the files or directories 
         owned by the filesystem or man package. If you feel that you have a 
         good reason to own a file or directory that another package owns, 
         then please present that at package review time.
[  OK  ] MUST: At the beginning of %install, each package MUST run rm -rf 
         %{buildroot} (or $RPM_BUILD_ROOT)
[  OK  ] MUST: All filenames in rpm packages must be valid UTF-8

- Consider using the following to preserve timestamps of non-created files:
  make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"

From my view as a packager, this package seems personally good to me as well,
thus: APPROVED.

Joe, apply or refuse my suggestion and close the bug report afterwards with
the hint which option you've chosen (it's a SHOULD, not more).

Oh, you may want to consider disttag, but it's only a SHOULD. As you've e.g. 
refused it for httpd, I'm assuming same here - but just to mention it.

This is very weird; I thought I reviewed this package something like two years ago, and indeed it seems that I did.

Yes, this was fedora-review+'ed by you but Rex cleared it and reopened the bug in comment 10 (see the history link).

Yeah, I don't see a need for disttag in the devel branch, and it screws up in some cases when it's there.  I've committed and the "install -p" fix - thanks a lot Robert for the re-review! :)

Closing out for good, hopefully...

1065558 build (dist-f11, /cvs/pkgs:rpms/neon/devel:neon-0_28_3-3) completed successfully