Bug 190991

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.