Bug 165265 - Review Request: libnjb
Review Request: libnjb
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michael Schwendt
David Lawrence
http://libnjb.sourceforge.net/
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-08-06 02:52 EDT by Linus Walleij
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-08-15 17:36:19 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Linus Walleij 2005-08-06 02:52:54 EDT
Spec Name or Url: libnjb.spec
SRPM Name or Url: http://sourceforge.net/project/showfiles.php?group_id=32528
Description: A library for accessing the Creative NOMAD and Nomad Zen
as well as Dell DJ jukebox devices.
Comment 1 Michael Schwendt 2005-08-06 09:05:28 EDT
[Where did the FE-NEW entry go?]
Comment 2 Linus Walleij 2005-08-06 10:49:18 EDT
No idea, trying to use this new process is a bit shaky I guess..
Comment 3 Ralf Corsepius 2005-08-07 00:24:47 EDT
# rpmlint /tmp/libnjb-2.2.1-3.src.rpm
W: libnjb hardcoded-packager-tag Linus

- Remove from the spec:
if [ -d $RPM_BUILD_ROOT ]; then rm -r $RPM_BUILD_ROOT; fi
This is not useful.
rm -rf $RPM_BUILD_ROOT
has the same effect


- Missing dependendies
# rpm -q --requires -p libnjb-devel-2.2.1-3.i386.rpm
libnjb = 2.2.1-3
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(VersionedDependencies) <= 3.0.3-1

Requires: ncurses-devel, zlib-devel, libusb-devel
are missing

- Many questionable conditionals inside of the spec.
Comment 4 Linus Walleij 2005-08-07 14:46:27 EDT
Thanks Ralf, your comments are much welcomed and your advice much
needed.

The Packager:-tag is only used conditionally if built for something
else than Fedora. (One of the two questionable conditionals.) rpmlint
obviously does not honour conditionals.

Regarding the conditionals: these have been introduced in accordance
with the dist-tag guidelines at http://www.fedoraproject.org/wiki/DistTag
in order to use the same .spec file for Mandrake, PLD and possibly SuSE.

If it is the view of the Fedora Extras project that conditionals may not
be used to prepare generic .spec files, this has to be stated in the
packaging guidelines and specifically in the dist-tag guidelines, because
the current text only triggers a programmer-type like me to do something
generic. I will not be the last one to do this mistake...

It should then say something like: "only use conditionals for
spec files intended for Fedora, Red Hat and RHEL, do not try to use
conditionals to cook generic spec files for other distributions, make 
unique spec files for each distribution instead". (If this is what is
implicitly meant, and then I will change the spec accordingly.)

The rest of the things are fixed, enjoy libnjb-2.2.1-4 on the same
URL (we are making progress, are we not?)
Comment 5 Michael Schwendt 2005-08-09 08:33:28 EDT
[Issue with FE-NEW has been fixed.]

[...]

The src.rpm libnjb-2.2.1-4.src.rpm didn't build here.
Adding "Buildrequires: doxygen" fixed it.

RPM build errors:
    File not found:
/home/qa/tmp/rpm/tmp/libnjb-2.2.1-4-root-qa/usr/share/doc/libnjb-2.2.1
    File not found:
/home/qa/tmp/rpm/tmp/libnjb-2.2.1-4-root-qa/usr/share/doc/libnjb-2.2.1/html
    File not found by glob:
/home/qa/tmp/rpm/tmp/libnjb-2.2.1-4-root-qa/usr/share/doc/libnjb-2.2.1/html/*

[...]

Some comments in the matter of reviewing: I agree with Ralf that the
conditional sections in the spec file introduce questionable things,
which make it somewhat tiresome and error-prone to review such an rpm.
Overuse of macro definitions or redefinitions of macros is another
source of mistakes.

Now, let me point out that only few reviewers would care much about
spec file formatting and optional switches (like --with/--without) or
even conditional sections. The packager is supposed to maintain his
package, not the reviewer. However, as soon as there is enough reason
to believe that a spec file is not straight-forward enough to build
correctly, and if it looks as if the packager will rewrite it
completely for the next update, a reviewer would approve it only
reluctantly. Better keep spec files short and to the point and
hence readable. You just want to package the software reliably, not
turn a spec file into a bloated piece of confusing code, which likely
has introduces increased maintenance requirements.

For instance, the following things are clearly wrong:

> Prefix:         %{_prefix}

Setting the "Prefix:" tag like this marks the package as being
relocatable. It can be proven easily that these packages are _not_
relocatable. Take a look at the contents of e.g. the pkgconfig
file. It contains hardcoded /usr paths. Argueing that the other
files of packages could be relocated nevertheless, would be possible,
but would not fix the package.

> $ rpmlint libnjb-devel-2.2.1-4.i386.rpm 
> E: libnjb-devel standard-dir-owned-by-package /usr/include
> E: libnjb-devel standard-dir-owned-by-package /usr/lib

Both directories (and the corresponding definitions of ownership
and access permissions) belong into the filesystem package only.

> Docdir:		%{prefix}/share/doc

%_docdir (and %_defaultdocdir) is predefined as /usr/share/doc
which is the same as %{_datadir}/doc

> %files
> %defattr(-, root, root)
> %dir %{_libdir}

This %dir statement is the source of the rpmlint error for one
of the directories, which must not be included in your package.
%_includedir as well as /etc/hotplug and /etc/hotplug/usb also
do not belong into your packages.


Other things:

> Source:		%{name}-%{version}.tar.gz

Sourceforge.net URLs of the form

http://download.sourceforge.net/%{name}/%{name}-%{version}.tar.gz
http://dl.sf.net/%{name}/%{name}-%{version}.tar.gz

give direct access to a download server mirror. Just a hint.

> %package examples
> Summary:        Example programs for libnjb
> Group:          System Environment/Libraries

Strange group for example binaries. Minor issue though.

> # Remove static library remnant
> rm -f $RPM_BUILD_ROOT%{_libdir}/libnjb.la

It's a libtool archive, not a "static library remnant".

> mkdir -p $RPM_BUILD_ROOT/etc/hotplug/usb

/etc is %{_sysconfdir} for those who think it may change ever.

> %post
> /sbin/ldconfig
> 
> %postun
> /sbin/ldconfig

Better:

%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig

That will make /sbin/ldconfig the scriptlet command interpreter
and create automatic dependencies on /sbin/ldconfig. Your version
would include a shell script, which runs /sbin/ldconfig.
Comment 6 Linus Walleij 2005-08-09 14:54:57 EDT
Thanks Michael, excellent feedback, as always.

Most things just fixed, a -5 release of the package is now available on the
same URL.  A few notes:

Dropped all conditionals: if a generic .spec file is to be done, one
shall do it in some other macro language and generate a Fedora .spec from it,
I conclude. (Will enter into the Wiki someday.)

I need some clear definition of what it means that a package is relocatable:
this library is not bound to /usr at compile time: autoconf:s "configure"
script process the pkgconfig.pc.in file to create a pkgconfig.pc file
in accordance with the --prefix given from RPM by way of the %configure
macro. Does this not mean that the package is relocateble? I was sort of
wondering if "relocatable" is to be understood as "relocatable after
packaging" not "relocatable at packaging time". I have successfully 
compiled and used this library with "configure --prefix=$HOME" for
example so it is indeed relocatable at compile time as far as I know
(and if there still is some issue, I can fix it) as far as you set the
apropriate search paths for libraries and pkgconfig. For example it can
be relocated to /opt/lib without any problems, many users have done this.

I fixed the hotplug dir owning issue by introducing a dependency on hotplug,
also noticed that the pkgconfig dirs used are better off as a 
Requires: pkgconfig line.

The .la files are mystically called static libs in the package review
guidelines, that's why that comment was so strange ( - MUST: Packages 
must NOT contain any .la static libraries, these should be removed in 
the spec.) I had no idea that extension signified a libtool archive,
just a vague idea of what it was. Good that I know now.

Weird with %{_docdir}: Tom Calloway wrote:

> In fact, everything that rpm is capable of defining as:
> 
> Foo:            bar
> 
> Will then exist as %{foo}.

But! %{docdir} is NOT defined, instead we have to use %{_docdir} which
is defined in the same magical fashion... Something is weird here and 
not quite logical. I set:

Docdir:    %{_defaultdocdir}

and use %{_docdir} in subsequent statements and hope for the best.
Comment 7 Michael Schwendt 2005-08-09 15:49:55 EDT
> I was sort of wondering if "relocatable" is to be understood as
> "relocatable after packaging" 

Yes, it is.

$ rpm -qpi libnjb-2.2.1-4.i386.rpm  | grep Reloc
Name        : libnjb                       Relocations: /usr 

Files starting with /usr can be relocated at install-time.
Also see "man rpm":

       --relocate OLDPATH=NEWPATH
              For relocatable binary packages, translate all file  paths  that
              start with OLDPATH in the package relocation hint(s) to NEWPATH.
              This option can be used repeatedly if several OLDPATH’s  in  the
              package are to be relocated.

> (and if there still is some issue, I can fix it) as far as you set the
> apropriate search paths for libraries and pkgconfig. For example it can
> be relocated to /opt/lib without any problems, many users have done this.

Basically, it's relocatable except for the pkgconfig file, which contains 
hardcoded references to /usr, which would break when installed in /opt e.g.
*That* plus the package dependencies on non-relocatable packages make the 
decision to mark libnjb as relocatable, a questionable decision. Another
pretty serious problem with relocatable packages is that you must keep them
relocatable forever, which is a common pitfall when a library starts
accessing data files in %_datadir, for instance.

> also noticed that the pkgconfig dirs used are better off as a 
> Requires: pkgconfig line.

It's a soft-requirement, since libnjb.so and headers are in standard search 
paths. It would be a hard-requirement, if an application were unlikely to
find libnjb and headers without using pkg-config.

> The .la files are mystically called static libs in the package review
> guidelines, that's why that comment was so strange 

Documentation bug.

> But! %{docdir} is NOT defined, instead we have to use %{_docdir} which

%_docdir is a global, no need to define it.

> I set:
>
> Docdir:    %{_defaultdocdir}

Not necessary either.
Comment 8 Linus Walleij 2005-08-09 16:29:42 EDT
OK then I guess we are all done because the -5 version is hardcoded to
/usr as suggested (and it is a good thing). %_docdir still evades me,
there is something fishy here:

[root@felicia SRPMS]# rpm --eval '%_docdir'
%_docdir

However after using Docdir: %{_defauldocdir} inside a specfile it exists 
and can be used. Or is it there, just not visible with --eval? Sorry 
if these questions are somewhat RTFM but I really read all packaging 
guidelines, Maximum RPMs and you know what, it still proves that 
lots of knowledge seemingly just comes from reading very many 
specfiles...

http://www.fedoraproject.org/wiki/Extras/RPMMacros does not mention
this macro, nor does Maximum RPMs, nor does the rpmbuild manpage etc.
RPM documentation is somewhat lagging behind.

As for the soft requirement: if I Require: pkgconfig I know I have the
dir %{_libdir}/pkgconfig but otherwise I must own that dir something
like

%dir %{_libdir}/pkgconfig
%files ...

right? So we have multiple owners of that dir in this case, creating
a soft dependency in case pkgconfig is not installed.
Comment 9 Michael Schwendt 2005-08-09 17:33:07 EDT
You are free to use %{_defaultdocdir} instead if you prefer that.

> However after using Docdir: %{_defauldocdir} inside a specfile
> it exists and can be used.

Again, %_docdir comes predefined. No need to define "Docdir:" something.
I just introduces mistakes and possibly influences things like the
following. Your %doc files are located in

  /usr/share/doc/libnjb-devel-2.2.1/

while your html files are located in

  /usr/share/doc/libnjb-2.2.1/

and both in one package.

> %dir %{_libdir}/pkgconfig

That directory does not belong into your package. And hence your package
should not own it. Either "Requires: pkgconfig" or leave the directory
unowned. Installing package pkgconfig _after_ libnjb-devel would even
overwrite the directory permissions.

I don't know how else to explain it. One last try: libnjb-devel works well 
without pkg-config, since library and headers are in standard search path.
Due to this installing and using pkg-config is optional. If you feel like
making it a requirement, add the "Requires: pkgconfig". That package exists
in Core, so nobody should complain if it were needed. However, to add the
dependency just to get the package that owns %_libdir/pkgconfig/ would be
exaggeration IMHO. 

Directory ownership _is_ important in other cases (and that's why it's
in the packaging guidelines). For directories like %_libdir/pkgconfig/ or
%_datadir/aclocal/ it's not. That's IMHO. YMMV.


> OK then I guess we are all done because the -5 version is hardcoded to
> /usr as suggested

Defining "Prefix: /something" still marks the package as relocatable.
Suggested fix: Don't define "Prefix:".


Such a small package, oh so many things to make it complicated. ;-)
Comment 10 Linus Walleij 2005-08-10 09:55:10 EDT
OK sorry I'm not always the bright guy I wish I was...

There is a -6 version of the SRPM now that hopefully and at
last ties up all loose ends.

I will have to roll something more after this so that I get used
to doing things The Right Way (TM). Thanks Michael.
Comment 11 Michael Schwendt 2005-08-11 12:33:27 EDT
Okay. Release 6 is good. Approved. (But please get rid of that Docdir in CVS.)

--- libnjb.spec~        2005-08-10 14:40:13.000000000 +0200
+++ libnjb.spec 2005-08-11 18:34:16.000000000 +0200
@@ -50,7 +50,6 @@
 Requires:      libusb-devel
 Requires:      zlib-devel
 Requires:      ncurses-devel
-Docdir:                %{_docdir}
 
 %description devel
 This package provides development files for the libnjb
Comment 12 Linus Walleij 2005-08-15 17:36:19 EDT
OK building and all. Closing.
Comment 13 Michael Schwendt 2005-08-16 02:10:19 EDT
Can you please provide a different e-mail address? Several people
have tried to contact you and failed like this:

From: MAILER-DAEMON@mail.gmx.net
Subject: failure notice
Date: 16 Aug 2005 06:01:32 -0000

Hi. This is the qmail-send program at mail.gmx.net.
I'm afraid I wasn't able to deliver your message to the following addresses.
This is a permanent error; I've given up. Sorry it didn't work out.

<triad@df.lth.se>:
194.47.250.12_does_not_like_recipient./Remote_host_said:_553_5.3.0_<triad@df.lth.se>..._Rejected_-_see_http://dnsbl.rangers.eu.org//Giving_up_on_194.47.250.12./
Comment 14 Linus Walleij 2005-08-16 13:48:35 EDT
OK sorry indeed for all the DNSBL trouble Michael, I had my postmaster
remove my account from any blacklist filtering. If I survive the incoming
spam flood with just spamassassin and ClamAV I'll keep it this way
and hope the mail gets through.

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