Spec URL: http://www.di.ens.fr/~rineau/Fedora/libpar2.spec SRPM URL: http://www.di.ens.fr/~rineau/Fedora/libpar2-0.2-1.src.rpm Description: PAR 2.0 compatible file verification and repair library libpar2 is a library for creating and using PAR2 files to detect damage in data files and repair them if necessary. PAR2 files are usually published in binary newsgroups on Usenet; they apply the data-recovery capability concepts of RAID-like systems to the posting and recovery of multi-part archives. libpar2 is a required by gpar2. Notice: I need a sponsor. As a matter of fact, this is not my first review request: par2cmdline (bug #190070) has been accepted, but i forgot to tell it was my first request. Actually, I discovered that par2cmdline code is two year old, while libpar2 (same authors) is still maintained and improved. I will submit of request for gpar2, a par2 graphical client, in a few minutes. I suggest that the two request are reviewed in a whole.
gpar2 request is bug #190992.
Some comments: 1. Minor issues: 1.1 # rpmlint *RPMS/libpar*.rpm E: libpar2-debuginfo script-without-shellbang /usr/src/debug/libpar2-0.2/galois.h E: libpar2-debuginfo script-without-shellbang /usr/src/debug/libpar2-0.2/par2repairersourcefile.cpp E: libpar2-debuginfo script-without-shellbang /usr/src/debug/libpar2-0.2/par2repairer.cpp E: libpar2-debuginfo script-without-shellbang /usr/src/debug/libpar2-0.2/par2repairersourcefile.h E: libpar2-debuginfo script-without-shellbang /usr/src/debug/libpar2-0.2/par1repairer.cpp E: libpar2-devel only-non-binary-in-usr-lib Most of these probably are caused by bogus permissions on source files. 1.2 Empty directory /usr/lib/libpar2 Is this package supposed to take plugins, there? 2. Major: This package's configuration (configure.ac/Makefile.am) is bugged: - configure.ac uses PKG_CHECK_MODULES(..sigc++..) but doesn't propagate the results to Makefile.am. Instead the Makefile explicitly links against -lstdc++. This violates g++'s working principles. Linking against -lstdc++ is a g++ internal detail. 3. Severe (Blocker): The package installs an autoheader (config.h) to a public directory (/usr/lib/libpar2/include/config.h). This file's contents will clash with other package's config headers and is a severe (must fix) design flaw of this package. A package must not install an autoheader.
Update: Spec URL: http://www.di.ens.fr/~rineau/Fedora/libpar2.spec SRPM URL: http://www.di.ens.fr/~rineau/Fedora/libpar2-0.2-2.src.rpm Thank you for your comments, Ralf. (In reply to comment #2) > 1. Minor issues: > > 1.1 > # rpmlint *RPMS/libpar*.rpm > E: libpar2-debuginfo script-without-shellbang I fixed these one. I forgot to apply rpmlint to the debuginfo package. > E: libpar2-devel only-non-binary-in-usr-lib I saw this one. I thought it is related to %{_libdir}/libpar2/include/config.h See my answer to your point 3. > Most of these probably are caused by bogus permissions on source files. > > 1.2 Empty directory > /usr/lib/libpar2 > Is this package supposed to take plugins, there? It should includes a subdirectory include/, with config.h in it. See point 3. > 2. Major: > This package's configuration (configure.ac/Makefile.am) is bugged: > - configure.ac uses PKG_CHECK_MODULES(..sigc++..) but doesn't propagate the > results to Makefile.am. Instead the Makefile explicitly links against -lstdc++. > This violates g++'s working principles. Linking against -lstdc++ is a g++ > internal detail. This is an upstream bug. I am not an automake guru (not yet). I added a patch in the package. It should correct this point. I'll try to make this patch accepted by upstream, as soon as somebody confirms that this patch is correct. > 3. Severe (Blocker): > The package installs an autoheader (config.h) to a public directory > (/usr/lib/libpar2/include/config.h). This file's contents will clash with other > package's config headers and is a severe (must fix) design flaw of this package. > A package must not install an autoheader. I do not understand this point. This file config.h is in a directory owned by the package: %{_libdir}/libpar2/include/ If a package cannot install an autoheader (such as config.h), how could dependencies access to the compilation options used to build the package? I have checked that several packages, some in FE, have config.h in %{_libdir}/%{name}/include: at least sigc++-2.0, gtkmm-2.4, glib-2.0, and gtk-2.0. I thought this was the usual way to process with config.h Can you confirm that this point is a blocker for this request? As far as I understand things, it does not seem too.
(In reply to comment #3) > > 3. Severe (Blocker): > > The package installs an autoheader (config.h) to a public directory > > (/usr/lib/libpar2/include/config.h). This file's contents will clash with > other > > package's config headers and is a severe (must fix) design flaw of this > package. > > A package must not install an autoheader. > > I do not understand this point. This file config.h is in a directory owned by > the package: %{_libdir}/libpar2/include/ > > If a package cannot install an autoheader (such as config.h), how could > dependencies access to the compilation options used to build the package? There is no general answer to this question, but "somehow" - This is the basic question package configuration tools want to solve. The fundamental design flaws with exporting config header are: * autoheaders generated config.h's contain a snapshot of the build system's state having been taken at the time the configure script had been run. This state is by no means connected to a system's state, the package had been installed on. * autoheaders contain defines that are reserved to autoconf and will clash with those autoconf internal defines being used by configure scripts wanting to use this library. (E.g. /usr/lib/libpar2/include/config.h's PACKAGE_NAME will do so) * autoheaders reflect a package's internal demands/requirements. These are not connected to a package "using a package"'s demands. AFAIS, libpar2 expects packages using it to provide a set of autoconf defines. This doesn't work. > I have checked that several packages, some in FE, have config.h in > %{_libdir}/%{name}/include: at least sigc++-2.0, gtkmm-2.4, glib-2.0, and > gtk-2.0. I thought this was the usual way to process with config.h If the files you are referring to are autoheaders, these packages are also bugged. libtiff is a well known case having suffered (still suffering?) from this issue. > Can you confirm that this point is a blocker for this request? Yes, it is. The sources are suffering from a design flaw which disqualify this package from FE.
Ralf, could you either indicate where in the guidelines this type of thing is forbidden or at least point to some discussion on the matter? Your final statement about the package being disqualified from Extras seems awfully arbitrary without it. I have no opinion one way or the other on this issue because I am not familiar with it, but if you're just making up policy then I don't think it's fair to the package submitter.
(In reply to comment #5) > Ralf, could you either indicate where in the guidelines this type of thing is > forbidden or at least point to some discussion on the matter? Your final > statement about the package being disqualified from Extras seems awfully > arbitrary without it. This is not a matter of taste nor of personal preference. It's a mere technical requirement implied by autoconf's and a compiler's working principles (cf. info autoconf, search the autoconf mailing lists archive). Or to put it in short: This package is unusable. > I have no opinion one way or the other on this issue because I am not familiar > with it, but if you're just making up policy then I don't think it's fair to > the package submitter. Sorry, but I am not making up packaging policies here nor am I accusing the packager. It's simply a case of "this package's sources are broken and need to be reworked to be functional".
(In reply to comment #6) > It's simply a case of "this package's sources are broken and need to be > reworked to be functional". Let me demonstrate the bug: cat par2bug.cc #include <libpar2/libpar2.h> // Fake using an autoheader #if HAVE_CONFIG_H #define PACKAGE_NAME foo #endif int main() { } 1) # g++ -o par2bug -DHAVE_CONFIG_H -I. $(pkg-config --cflags sigc++-2.0) \ $(pkg-config --libs sigc++-2.0) -lpar2 par2bug.cc In file included from /usr/include/libpar2/libpar2.h:5, from par2bug.cc:1: /usr/include/libpar2/par2cmdline.h:66:20: error: config.h: No such file or directory => par2cmdline.h expects the package using libpar2 to provide config.h 2) # g++ -o par2bug -DHAVE_CONFIG_H -I. $(pkg-config --cflags sigc++-2.0) \ $(pkg-config --libs sigc++-2.0) -lpar2 -I/usr/lib/libpar2/include par2bug.cc par2bug.cc:4:1: warning: "PACKAGE_NAME" redefined In file included from /usr/include/libpar2/par2cmdline.h:66, from /usr/include/libpar2/libpar2.h:5, from par2bug.cc:1: /usr/lib/libpar2/include/config.h:98:1: warning: this is the location of the previous definition => clash on autoheader define PACKAGE_NAME
Actually, here is a list of packages that include a generated config header, on my FC-5¹: gd-devel glibmm24-devel gtkmm24-devel gtkmm2-devel libjpeg-devel libsigc++20-devel libsigc++-devel libtiff-devel openldap-devel postgresql-devel Some come from FC-5 and other from FE-5. ¹) generated with: rpm -qf $(grep -l Generated /usr/lib*/*/include/*.h /usr/include/*.h| grep -v qt-3) | sort -u However, I agree that autoheaders should not be installed in the system. Perhaps should we fill a bug report for these different packages. I am very willing to have libpar2 and gpar2 in FE. Can you explain what could be a solution of a library that need autobuild tools to define some object types and operations? libpar2 needs to know the endianess to define types leu16, leu32, and leu64 (see letype.h). These three types are equal to uint16_t, uint32_t and uint64_t, on little endian platforms, but are classes with cast operations on big endian platforms. What mechanism could use libpar2 to avoid installing autoheader, so that letype.h is correct?
(In reply to comment #8) > Actually, here is a list of packages that include a generated config header, > on my FC-5¹: > Some come from FC-5 and other from FE-5. OT at this point - This is libpar2's review request. If you think the packages you listare broken, file individual PRs against them. But beware, your list seems to contain many false positives. > However, I agree that autoheaders should not be installed in the system. > Perhaps should we fill a bug report for these different packages. Exactly - But make sure to have understood the issue. The problem is NOT packages shipping generated headers, the problem is packages shipping autoheaders (headers generated by "autoheader"). > I am very willing to have libpar2 and gpar2 in FE. Can you explain what could > be a solution of a library that need autobuild tools to define some object > types and operations? Very hard to answer. AFAIS, libpar2 suffers from a fundamentally broken API. > libpar2 needs to know the endianess to define types > leu16, leu32, and leu64 (see letype.h). These three types are equal to > uint16_t, uint32_t and uint64_t, on little endian platforms, but are classes > with cast operations on big endian platforms. What mechanism could use libpar2 > to avoid installing autoheader, so that letype.h is correct? IMO, upstream should redesign the API (Keywords data abstraction/encapsulation). Somewhat less fundamentally, upstream should find a way to hard-code the defines it relies upon into an exported header, instead of letting its exported headers depend upon external programs. A very brutal, short-term workaround/hack would be to kick out par2cmdline.h and to replace it with a file using hard-coded values.
I eventually decided to cancel this review request for a while. libpar2 is not ready to be part of Fedora Extras. I am in touch with upstream developers, who agreed that libpar2 is a dirty adaptation of par2cmdline, in order to build gpar2. I will work with them and reopen this request libpar2 and gpar2 latter.
Some of the "discoveries" in this ticket are wrong and misleading. The biggest problem with "config.h" (regardless of its contents!) is global namespace pollution when installing the file into a location that takes precedence in the search-path list. The observation that auto-generated headers are wrong/bad in general, is wrong. Surely there are valid scenarios in which it makes sense to re-use generated headers in a public API. Finally, I fail to see where libsigc++-2.0 and gtkmm-2.4 are affected by this.
(In reply to comment #11) > The observation that auto-generated headers are wrong/bad in general, > is wrong. Surely there are valid scenarios in which it makes sense to > re-use generated headers in a public API. Then I have to correct you: Installing files generated by "autoheader" is always wrong. They are not *designed* for this purpose. Any program doing so is abusing them.
"auto-generated headers" != "using autoheader"
(In reply to comment #13) > "auto-generated headers" != "using autoheader" Exactly.