Bug 165265
Summary: | Review Request: libnjb | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Linus Walleij <triad> |
Component: | Package Review | Assignee: | Michael Schwendt <bugs.michael> |
Status: | CLOSED NEXTRELEASE | QA Contact: | David Lawrence <dkl> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-package-review |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
URL: | http://libnjb.sourceforge.net/ | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2005-08-15 21:36:19 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 |
Description
Linus Walleij
2005-08-06 06:52:54 UTC
[Where did the FE-NEW entry go?] No idea, trying to use this new process is a bit shaky I guess.. # 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. 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?) [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. 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.
> 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. 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. 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. ;-) 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. 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 OK building and all. Closing. Can you please provide a different e-mail address? Several people have tried to contact you and failed like this: From: MAILER-DAEMON.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.se>: 194.47.250.12_does_not_like_recipient./Remote_host_said:_553_5.3.0_<triad.se>..._Rejected_-_see_http://dnsbl.rangers.eu.org//Giving_up_on_194.47.250.12./ 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. |