Spec URL: http://fedorapeople.org/~brouhaha/libbsd/libbsd.spec SRPM URL: http://fedorapeople.org/~brouhaha/libbsd/libbsd-0.2.0-1.fc12.src.rpm Koji scratch build for f12-dist: http://koji.fedoraproject.org/koji/taskinfo?taskID=1951266 Description: libbsd provides useful functions commonly found on BSD systems, and lacking on others like GNU systems, thus making it easier to port projects with strong BSD origins, without needing to embed the same code over and over again on each project.
A few notes: - Here you don't need to do anything that requires knowing the exact SOversion, so you can safely drop the SOlib definitions. Just hard-code the version to 0.2.0 and use wildcards %{name}.so.* in %files. - Does the library *really* have hard-coded paths in the source code? If not, then you can drop libdir=%{_libdir} usrlibdir=%{_libdir} exec_prefix=/usr" from the make command. - Use %{_prefix} instead of /usr. - If you remove nlist.h, then you should require the package that provides it. Are the files really compatible? - You should own the directory %{_includedir}/bsd/. - The devel package should Requires: pkgconfig, if you are going to build for EPEL, modern Fedoras pick up the requirement automatically.
> you can safely drop the SOlib definitions. OK > Does the library *really* have hard-coded paths in the source code? If not, > then you can drop libdir=%{_libdir} usrlibdir=%{_libdir} exec_prefix=/usr" from > the make command. Can't drop them. The make uses sed to substitute the directory paths into the .pc file. > Use %{_prefix} instead of /usr. OK > You should own the directory %{_includedir}/bsd/. OK > The devel package should Requires: pkgconfig, if you are going to build for EPEL, modern Fedoras pick up the requirement automatically. OK > If you remove nlist.h, then you should require the package that provides it. > Are the files really compatible? This one is tricky. The functionality of nlist in libbsd is intended to be identical to that in elfutils-libelf. To at least a superficial examination, the header seems functionally identical. However, I'm not 100% certain that the libbsd implementation is suitable for Fedora Linux. Anyone that depends on nlist should use the one from elfutils-libelf. Since libbsd is just a collection of random BSD stuff, it seems likely that the vast majority of libbsd users won't use nlist. I don't think omitting nlist makes it appropriate to have the package depend on elfutils-libelf. Adding that dependency will not solve the problem even for a libbsd user that does want nlist, since they would need to add a the elfutils-libelf to their link anyhow. The other reason that I don't think omitting nlist from libbsd is going to be a serious problem for anyone is that the only purpose of libbsd is to support porting programs from BSD, and anyone doing that will have to change the #include directives in the program being ported. What's more of a concern is that if someone does link to both libbsd and elfutils-libelf, they may get the wrong nlist implementation depending on the link order. I think it's best to change the libbsd makefile to not compile or link nlist, as well as omitting the header. Unless someone has a serious objection, I'm going to do that tomorrow and make a new spec and SRPM available for review.
(In reply to comment #2) > > If you remove nlist.h, then you should require the package that provides it. > > Are the files really compatible? Actually, I feel this package is not really close to what it intends to be. > This one is tricky. Not really. Simply install this library's headers into a subdirectory of %{_includedir}, say %{_includedir}/libbsd, instead of %{_includedir}. This would also help avoiding such potential inclusion issues this package is not unlike to be suffering from.
> Actually, I feel this package is not really close to what it intends to be. What do you mean? As far as I can tell, it's supposed to be a random assortment of BSD library functions that tend not to be found on non-BSD systems, and that certainly seems to be what it is. The reason I'm packaging libbsd is that I want to package a program that uses strlcpy, and when I asked on the devel list about the best practice for doing that, Tom "spot" Callaway suggested using libbsd. It certainly seems like a better approach than dumping some random implementation of strlcpy into the package, or hacking the package to not need strlcpy. At least for this there's a maintained upstream. > Not really. Simply install this library's headers into a subdirectory of > %{_includedir} Many of them were already in %{_includedir}/bsd; I've now moved nlist.h there as well. Given that the purpose is to provide library functions for which we don't have a native equivalent, I don't really see the point, as it would be much better for ported apps to use the native version, but it was certainly easy enough to do. Here's the updated spec etc: Spec URL: http://fedorapeople.org/~brouhaha/libbsd/libbsd.spec SRPM URL: http://fedorapeople.org/~brouhaha/libbsd/libbsd-0.2.0-2.fc12.src.rpm Koji scratch build for f12-dist: http://koji.fedoraproject.org/koji/taskinfo?taskID=1952087
(In reply to comment #4) > > Actually, I feel this package is not really close to what it intends to be. > > What do you mean? I feel this package is immature and suffers from an inconsistent design. E.g. it provides a) /usr/include/vis.h It is adding a bsd-header to glibc. b) /usr/include/bsd/*.h These files only are usable if being explicitly included, e.g. #include <bsd/string.h> => inconsistent design. Also, it's not clear to me, if -I/usr/include/bsd ... (overriding system headers with bsd headers) is supposed to work. I guess, no, because a) from the list above doesn't match with this kind of usage. AFAIS, the contents of the files below bsd/ seem to be inconsistent wrt. this kind of usage. > > Not really. Simply install this library's headers into a subdirectory of > > %{_includedir} > Many of them were already in %{_includedir}/bsd I sense a misunderstanding. I recommend you to install this packages headers into a subdirectory of %{_includedir}, e.g. /usr/include/libbsd, such that you would end up with /usr/include/libbsd/vis.h ... /usr/include/libbsd/bsd/getopts.h ... /usr/include/libbsd/bsd/sys/cdefs.h ... This gets this library's headers out of gcc's default include search path and circumvents potential conflicts with glibc. Users would explictily have to use -I/usr/include/libbsd to get libbsd headers pulled in.
Regarding /usr/include/vis.h "adding a bsd-header to glibc", I strongly disagree. There is no general concept that headers in /usr/include belong to glibc. On my build system, there are 304 headers in /usr/include (not counting subdirectories), of which 109 are owned by glibc-headers, and 197 are owned by non-glibc packages. Aside from nlist,.h which I've moved, and vis.h which you mentioned, libbsd has one other header in /usr/include, libutil.h. Neither vis.h nor libutil.h conflict with any other Fedora package. Regarding /usr/include/libbsd, I understand what you suggested, but I disagree about the location. libbsd installs most of its headers in /usr/include/bsd, which exist in any other Fedora package. gcc reports that its default search path does not include /usr/include/bsd. Since the purpose is portability, I don't see any reason why moving those headers to /usr/include/libbsd is an improvement. In the case of nlist, which they installed as /usr/include/nlist.h, I've agreed to move it to /usr/include/bsd/nlist.h, which shouldn't be a problem for programs being ported since they should just use elfutils-libelf for that anyhow. Reading your comments I almost get the impression that you think libbsd is intended to be a general-purpose library that should eventually be relied on by newly written software and many Fedora packages, and nothing could be further from the truth. The point is merely to make it easier to build BSD-centric software for Fedora, without adding entirely new ad-hoc implementations of some of those functions into each such package, and having to maintain each of those. For instance, I could certainly write a strlcpy() function of my own to include in another package I'm porting from BSD, or I could patch the upstream source to not use strlcpy(), but in either case I am likely making the software LESS reliable by adding unproven code, and making them require MORE work to maintain in Fedora. Anyhow, while I'm perfectly willing to discuss the implications, at the end of the day I'm looking for a review that determines whether the package meets the Fedora packaging guidelines, not whether it is the finest, most elegant example of software engineering, nor whether strlcpy() and friends are good, bad, or ugly. The reality is that there are some packages out there written specifically for BSD, and it is useful to have a library that makes it easier to port them. No, libbsd doesn't solve all BSD portability problems, nor does it claim to.
Cut-and-paste error. I wrote > libbsd installs most of its headers in /usr/include/bsd, > which exist in any other Fedora package. That should have read "which does not exist in any other Fedora package."
(In reply to comment #6) > Regarding /usr/include/vis.h "adding a bsd-header to glibc", Then let me rephrase my remark: By adding this header to /usr/include this package attempts to convert a non-bsd system into a bsd compatible one. It avoids to do so by with the headers below bsd/ It fails to do with nlist.h That said, I impression is: This package is conceptionally immature and therefore must not install its headers directly into /usr/include > Reading your comments I almost get the impression that you think libbsd is > intended to be a general-purpose library that should eventually be relied on by > newly written software and many Fedora packages, This aspect doesn't matter. At the very point a devel package is installed it "is around", i.e. is can be used for arbitrary purposes, no matter what you or upstreams has intended. > No, libbsd doesn't solve all BSD portability problems, nor does > it claim to. This is not my point. My point is: I feel this package is not ready for public consumption. Me asking you to install its headers into a subdirectory is an attempt to isolate this package (e.g. from accidental use) and to prevent it to cause potential damage.
> It fails to do with nlist.h I already moved nlist.h into /usr/include/bsd. I don't know why we're still arguing about that one. > At the very point a devel package is installed it > "is around", i.e. is can be used for arbitrary purposes, By a conscious choice of a developer, certainly. That is, in fact, the purpose of -devel subpackages. > Me asking you to install its headers into a subdirectory is an > attempt to isolate this package (e.g. from accidental use) and to prevent it to > cause potential damage. You've failed to explain how the headers /usr/include/vis.h and /usr/include/libutil.h can be "accidentally used". When I write software, I don't throw in #includes of random headers I don't deliberately intend to use. The fact that no Fedora package provides or uses /usr/include/vis.h or /usr/include/libutil.h seems sufficient to ensure that no one is going to use libbsd without intending to do so. > I feel this package is not ready for public consumption. If I had some idea of how the "public" might accidentally "consume" these two headers, I'd be a lot less reluctant to move them.
FYI, you'll probably want to use -isystem /usr/include/bsd rather than -I /usr/include/bsd.
> you'll probably want to use -isystem /usr/include/bsd rather than > -I /usr/include/bsd. In general that may be a reasonable thing to do, but I think in the vast majority of cases -I /usr/include/bsd will be perfectly acceptable. If you're using pkgconfig to get the options, you'll get -I.
And I don't see how /usr/include/vis.h and /usr/include/libutil.h would be a problem: * they don't conflict with any other package in Fedora, * they are implementations of the headers usually named that way, not some unexpected conflicting headers squatting the names, * the "inconsistency" is because obviously you can't replace standard glibc headers with BSD ones. That said, -isystem /usr/include/bsd is not going to work as expected because the headers don't include the glibc ones. This is bad for portability, you have to patch programs to add extra #include statements. IMHO, the headers should be doing: #include_next <string.h> (or the respective header, of course) so you can use -isystem /usr/include/bsd. One solution could be to provide a separate /usr/include/libbsd which provides headers of the form: #include_next <string.h> #include <bsd/string.h> That way your libbsd package is compatible with upstream, but you can also easily port existing BSD programs with -isystem /usr/include/libbsd.
I don't mind having to change the includes a bit. In fact, I prefer it in order for it to be obvious that the patched sources are depending on libbsd. I'd rather leave them the way they are, unless upstream makes that change. If you feel strongly about it, you might want to suggest it in their bugzilla: https://bugs.freedesktop.org/
Upstream states that nlist.h is a problem in Debian as well, and actually recommends not including the header in the RPM. Removing the function from the library is not recommended as doing so would change the ABI, so upstream will remove it at the next SOVERSION bump. At this point since I've already moved the header out of harm's way per Ralf's request, I'm inclined to leave it in the RPM until it is removed upstream. It is also mentioned that nlist does not have a link order issue since libbsd and elfutils-libelf are both versioned. For more detail, see this message on the libbsd mailing list: http://lists.freedesktop.org/archives/libbsd/2010-January/000033.html
Alright, I think I'm going to take on this one.
* MUST: rpmlint must be run on every package. The output should be posted in the review. -- NEEDSWORK [sebastian@localhost SRPMS]$ rpmlint libbsd-0.2.0-2.fc12.src.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. [sebastian@localhost i686]$ rpmlint libbsd-* libbsd-devel.i686: W: file-not-utf8 /usr/share/man/man3/flopen.3.gz 3 packages and 0 specfiles checked; 0 errors, 1 warnings. * 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. -- N/A * 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. -- OK c6d5413e76949b14e4bf13258e63d355 libbsd-0.2.0.tar.gz * MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. -- OK * 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. -- N/A * 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. -- OK * MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden. -- N/A * 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. -- OK * MUST: Packages must NOT bundle copies of system libraries. -- OK * 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. -- N/A * 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 Fedora package must not list a file more than once in the spec file's %files listings. -- 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. -- OK * 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). -- N/A * 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. -- OK * MUST: Static libraries must be in a -static package. -- N/A * 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. -- OK * 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. -- N/A * 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. -- NEEDSWORK * SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. -- N/A * SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. -- N/A * SHOULD: The reviewer should test that the package builds in mock. -- OK * SHOULD: The package should compile and build into binary rpms on all supported architectures. -- N/T * SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example. -- N/T * SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. -- OK * SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. -- OK * SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb. -- OK * SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. -- N/A * SHOULD: your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense. -- OK COMMENTS: The package doesn't seem to contain a general license file, but since it has per-file license headers and this would only be a "should" item, you're good. You might want to query upstream at some point for inclusion of such a file, but this isn't a blocker. Please don't put the man pages in %doc. I can't find those guidelines right now, but from looking at CVS, for example parted doesn't do it, either: http://cvs.fedoraproject.org/viewvc/devel/parted/parted.spec?view=markup rpmlint isn't calm since one file name isn't UTF-8. You can easily fix this by running http://fedoraproject.org/wiki/Common_Rpmlint_issues#file-not-utf8 on the flopen.3 file in %prep. Besides that, it looks all good to me. Fix the man page location and recode the one file to UTF-8 and I'll approve it.
In reviews of other packages, I've been told that whether man pages should be marked as %doc is a matter of taste. However, I don't feel strongly about it so I have removed the %doc per your request. The requirement you marked as NEEDWORK, "MUST: All filenames in rpm packages must be valid UTF-8", concerns the filename, not the content. The filename in question is a valid UTF-8 filename. I don't like modifying the content of the package, but it does seem harmless to "fix" the encoding of a man page, so I have done so. Thanks for the review! Spec URL: http://fedorapeople.org/~brouhaha/libbsd/libbsd.spec SRPM URL: http://fedorapeople.org/~brouhaha/libbsd/libbsd-0.2.0-3.fc12.src.rpm Koji scratch build for f12-dist: http://koji.fedoraproject.org/koji/taskinfo?taskID=1967157
This looks good and is thus APPROVED.
New Package CVS Request ======================= Package Name: libbsd Short Description: Library providing BSD-compatible functions for portability Owners: brouhaha Branches: F-12 InitialCC:
CVS done (by process-cvs-requests.py).
libbsd-0.2.0-3.fc12 has been submitted as an update for Fedora 12. http://admin.fedoraproject.org/updates/libbsd-0.2.0-3.fc12
libbsd-0.2.0-3.fc12 has been pushed to the Fedora 12 stable repository. If problems still persist, please make note of it in this bug report.
Package Change Request ====================== Package Name: libbsd New Branches: el6 Owners: brouhaha
Git done (by process-git-requests).
libbsd-0.3.0-2.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/libbsd-0.3.0-2.el6
libbsd-0.3.0-2.el6 has been pushed to the Fedora EPEL 6 stable repository.
Package Change Request ====================== Package Name: libbsd New Branches: el5 Owners: brouhaha
Package Change Request ====================== Package Name: libbsd New Branches: epel7 Owners: brouhaha