This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 168339 - Review Request: libbinio
Review Request: libbinio
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ralf Corsepius
David Lawrence
http://libbinio.sourceforge.net/
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-09-15 01:25 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-10-11 14:51:26 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-09-15 01:25:26 EDT
Spec Name or Url: http://www.df.lth.se/~triad/krad/fc/libbinio.spec
SRPM Name or Url: http://www.df.lth.se/~triad/krad/fc/libbinio-1.3-1.src.rpm

Description:

This binary I/O stream class library presents a platform-independent
way to access binary data streams in C++. The library is hardware 
independent in the form that it transparently converts between the 
different forms of machine-internal binary data representation.
It further employs no special I/O protocol and can be used on
arbitrary binary data sources.
Comment 1 Ralf Corsepius 2005-09-15 02:56:34 EDT
NEEDSWORK:

* BuildRequires:  libstdc++-devel
  ^^^^ This is probably superfluous.

* BuildRequires:  info
  ^^^^ This is probably wrong.
info is an *.info reader. *.infos normally are generated by makeinfo (from the
texinfo packages). Packages complying to the GNU Standards ship prebuilt *.infos
(Which AFAIS applies, here), so neither info nor texinfo should be required for
building.

* %{_includedir}/*.h
I hate packages installing their headers to /usr/include, because they pollute
the system include path. Instead, I recommend to install them to a subdirectory
of /usr/include, e.g.
%configure --includedir=%{_include}/binio
...
%{_includedir}/binio
...

* Your %post script is non functional.
You can't use -p /sbin/ldconfig there because you are trying to install infos at
the same time. You'll have to use something along these lines:

Requires(post): /sbin/ldconfig
Requires(post): /sbin/install-info

%post
/sbin/ldconfig
/sbin/install-info .....
Comment 2 Linus Walleij 2005-09-15 04:32:00 EDT
Thanks for the splendid review Ralf (as always).

New source package is created:
Spec Name or Url: http://www.df.lth.se/~triad/krad/fc/libbinio.spec
SRPM Name or Url: http://www.df.lth.se/~triad/krad/fc/libbinio-1.3-2.src.rpm

The BuildRequires: info was for the /sbin/install-info script that is used
in the build process, I have clarified this by changing it to
BuildRequires: /sbin/install-info. The rest is fixed according to your
comments.
Comment 3 Ville Skyttä 2005-09-15 10:35:04 EDT
Modifying the path where the headers are installed in a package which doesn't  
have a foo-config script or a pkgconfig file (which would adjust to correspond  
with the path where the headers are actually installed in the package) causes  
annoying extra work for packagers and others using such a -devel package,  
needs special treatment in all dependent packages, and should be thus strongly  
discouraged IMO.  If there's a foo-config or a *.pc file that would reflect  
the change, it'd be ok. 
 
On a related note, could add "|| :" at the end of those install-info commands 
in order to support --excludedocs installs. 
Comment 4 Ralf Corsepius 2005-09-15 15:07:23 EDT
(In reply to comment #3)
> Modifying the path where the headers are installed in a package which doesn't  
> have a foo-config script or a pkgconfig file (which would adjust to correspond  
> with the path where the headers are actually installed in the package) causes  
> annoying extra work for packagers and others using such a -devel package,  
What is so difficult to use
gcc -I/usr/include/binio .... ?!?

> needs special treatment in all dependent packages, and should be thus strongly  
> discouraged IMO.
I could not disagree more. IMO, all packages which install header files directly
to /usr/include are badly designed considered to be rejected.
Comment 5 Ville Skyttä 2005-09-15 15:54:37 EDT
(In reply to comment #4) 
> What is so difficult to use 
> gcc -I/usr/include/binio .... ?!? 
 
Well, it wouldn't be difficult to install the libs (or eg. "gcc", or whatever) 
into a custom path and to ask everyone to deal with it either.  That doesn't 
make it the right thing to do. 
 
See comment 3.  When done _by a packager on "IMO" basis_, it's (most likely, 
unchecked in this particular case) completely unnecessary, bloats dependent 
packages, possible maintenance burden, results in projects needing distro 
specific adaptations, not what upstream intended, and unexpected in general. 
 
I didn't say that dropping everything into /usr/include is optimal either.  
But non-transparent changes like this should not be made unilaterally by 
packagers, but rather reported/fixed upstream (preferably fixing it with a 
foo-config and/or a pkgconfig file) if something causes real world problems, 
or if someone cares deeply enough about it otherwise and can convince 
upstream. 
Comment 6 Linus Walleij 2005-09-15 16:48:00 EDT
OK then, two mentors say different things and I get to decide. Ville wrote
large parts of the Maximum RPM book so his karma is better, and I therefore
restore the .h files to the %{_includedir}.

New source package is created:
Spec Name or Url: http://www.df.lth.se/~triad/krad/fc/libbinio.spec
SRPM Name or Url: http://www.df.lth.se/~triad/krad/fc/libbinio-1.3-3.src.rpm

These magic "|| :" things - if I am correctly informed it is just a
way of using bash operators to avoid stop of the execution flow.
Isn't there something less magic than this "|| :" for example an
environment variable that can be IF:ed like this:

if [ $excludedocs = 1 ]; then
/sbin/install-info ....
fi

I have however added them so you can safely install with --excludedocs.
Am I approved then...
Comment 7 Ralf Corsepius 2005-09-15 22:33:07 EDT
(In reply to comment #5)
> (In reply to comment #4) 
> > What is so difficult to use 
> > gcc -I/usr/include/binio .... ?!? 
>  
> Well, it wouldn't be difficult to install the libs (or eg. "gcc", or whatever) 
> into a custom path and to ask everyone to deal with it either. 
GCC already does do this automatically ;)

> See comment 3.  When done _by a packager on "IMO" basis_, it's (most likely, 
> unchecked in this particular case) completely unnecessary, bloats dependent 
> packages, possible maintenance burden, results in projects needing distro 
> specific adaptations, not what upstream intended, and unexpected in general. 
BS - All packaging is based technical requirements AND on personal views.
Also, it's packagers who package set the per-package conventions for each
package. And it's the package users who have to adopt these per-package conventions.

> I didn't say that dropping everything into /usr/include is optimal either.
> But non-transparent changes like this should not be made unilaterally by 
> packagers, but rather reported/fixed upstream (preferably fixing it with a 
> foo-config and/or a pkgconfig file) if something causes real world problems, 

Any file in /usr/include causes real problems, because any file in /usr/include
* is likely to clash with standards (POSIX etc.)
* is likely to clash with other packages (other packages shipping headers with
the same name)
* prevents parallel installation.
* /usr/include is a privileged directory (system include directory) and is
treated special by compilers (secondary include path).

> or if someone cares deeply enough about it otherwise and can convince 
> upstream. 
If something clashes, it's too late for upstream to change.

Also, the main reason, why packages install headers into /usr/include is the
developers typically installing packages into private directories or into per
package directories, but not systemwide.

I will not approve any package which installs headers directly to /usr/include.
Comment 8 Linus Walleij 2005-09-16 06:33:16 EDT
OK so how are these disputes normally resolved?

I have no problem with doing what Ralf says, because the package that
I am creating to use libbinio do not complain about having its headers
in /include/binio so that is OK with me.

Ralf says he will veto against this package unless I do so, so Ville,
will you also veto against it in case I do so that it is a catch22
situation, or can you live with that I follow Ralfs demand?

Will it be brought up to the steering committe othwerwise?
Comment 9 Linus Walleij 2005-09-16 15:20:37 EDT
Sorry Ralf if I was misreading the diplomatic tone of considering vetos...
Anyway, I propose the following solution:

1. There is a new package,
   http://www.df.lth.se/~triad/krad/fc/libbinio-1.3-4.src.rpm
   which restores the relocation to %{_includedir}/binio

2. I will only build this for devel and FC-4 due to problems with GCC
   3.x (it does not descend into subdirs for finding headers, which means
   it would need lots of #include <foo.h> to be patched to 
   #include <binio/foo.h>). When upstream fixes this problem, we will
   consider packages for FC-3 and below to be built.

3. I will notify upstream that we want this to be default behaviour,
   if it is easy enough, I will also prepare a patch.

4. Ralf approves the package for devel and FC-4.

Is everybody happy with this solution?
Comment 10 Linus Walleij 2005-09-16 15:47:34 EDT
Upstream has been notified on the issue, see:
http://sourceforge.net/tracker/index.php?func=detail&aid=1293200&group_id=69728&atid=525584
Comment 11 Michael Schwendt 2005-09-17 05:00:16 EDT
I don't understand point 2.  First of all, any program which uses libbinio
ought to use '#include "binio/foo.h"' or '#include <binio/foo.h>"' anyway,
emphasising that these are non-standard headers. Else the fun starts when
the headers are installed into %_includedir/binio on one distribution and
%_includedir/libbinio on another one.
Secondly, for any version of GCC to find the headers in %_includedir/binio,
you need to add %_includedir/binio to the compiler's search path list, i.e.
-I/usr/include/binio for any programs which do '#include <binfile.h>',
pretending that it would be a standard header or a header in standard
search path.

As a another voice in this matter, I consider any program which cannot
be configured easily to extend the search path list for headers as broken.
You should always be able to build a program with libbinio installed into
/home/foo/libbinio/include (and /home/foo/libbinio/lib), for instance,
without having to do apply a lot of patch-work.
Comment 12 Ville Skyttä 2005-09-17 05:46:21 EDT
I'm not going to veto this change, but my opinion has not changed, and in any 
case, I suggest waiting for upstream comments before approving/pushing the 
package..
 
Re comment 6: 
 
- s/large parts/some bits here and there/ 
 
- "|| :" like in this case is just a cheap, generic way to ensure that the 
%post* etc scriptlet does not fail.  I'm not aware of a way to specifically 
detect an --excludedocs install.  One could check if the %{_infodir}/foo.info* 
files exist before invoking install-info on them though, but IMO "|| :" is 
very much sufficient in this case. 
 
Re comment 7: 
 
Forcing end users to adapt to per-package decisions instead of making things 
Just Work as expected out of the box significantly decreases the value of 
packaged software. 
Comment 13 Linus Walleij 2005-09-18 10:11:41 EDT
The problem with this package has been fixed, due to the much responsive
and understanding upstream author named Simon Peter. libbinio now installs
headers in %{_includedir}/libbinio/ (not binio - there is according to Simon
already an (much outdated) GNU package with this name).

The new upstream release also adds a pkgconfig libbinio.pc file to the
%{_libdir}/pkgconfig directory.

New package and SPEC:
Spec Name or Url: http://www.df.lth.se/~triad/krad/fc/libbinio.spec
SRPM Name or Url: http://www.df.lth.se/~triad/krad/fc/libbinio-1.4-1.src.rpm

Another quality fix suggested by the Fedora project! Everybody should 
be happy now. (Atleast for the time being.)
Comment 14 Ralf Corsepius 2005-09-18 10:21:03 EDT
(In reply to comment #13)
> New package and SPEC:
> Spec Name or Url: http://www.df.lth.se/~triad/krad/fc/libbinio.spec

%{_includedir}/%{name}/*.h
^^^
this should be 
%{_includedir}/%{name}
Comment 15 Linus Walleij 2005-09-18 10:49:44 EDT
Uh um I don't quite get it but does that syntax mean "own the directory and
everything in it" then I broke it down to the more obvious:

%dir %{_includedir}/%{name}
%{_includedir}/%{name}/*.h

Is this OK?
Spec Name or Url: http://www.df.lth.se/~triad/krad/fc/libbinio.spec
SRPM Name or Url: http://www.df.lth.se/~triad/krad/fc/libbinio-1.4-2.src.rpm
Comment 16 Michael Schwendt 2005-09-18 11:33:11 EDT
> %dir %{_includedir}/%{name}
> %{_includedir}/%{name}/*.h

Right.

Where you want to include the directory and its contents recursively,
the following one is a good recommendation:

%{_datadir}/%{name}/

The slash at the end makes it more clear that it is a directory and
not just a file.
Comment 17 Linus Walleij 2005-09-19 14:43:00 EDT
Could this be approved then?
Comment 18 Linus Walleij 2005-09-21 07:37:06 EDT
All NEEDSWORK on this package has, to the best of my knowledge, been
completed. Would someone please approve it?
Comment 19 Linus Walleij 2005-09-27 07:22:59 EDT
Would someone like to FE-ACCEPT (Approve) this package now?
Comment 20 Ralf Corsepius 2005-09-27 09:09:19 EDT
(In reply to comment #19)
> Would someone like to FE-ACCEPT (Approve) this package now?
First of all, you can't push me or any body else around here - Most of us are 
volunteers and probably only very few are being paid, so .. patience please ;)

Anyway, further comments:

There still are blocking issues:

1. the /usr/share/info/dir entry being produced by the *.texi is broken. 
(Try "info libbinio" and try to enter the libbinio node).

A fix to the sources would be this patch and to regenerate the info files:

diff -ur libbinio-1.4.orig/doc/libbinio.texi libbinio-1.4/doc/libbinio.texi
--- libbinio-1.4.orig/doc/libbinio.texi 2004-08-18 21:49:11.000000000 +0200
+++ libbinio-1.4/doc/libbinio.texi      2005-09-27 14:43:23.000000000 +0200
@@ -27,7 +27,7 @@

 @dircategory Software Libraries
 @direntry
-* libbinio: (libbinio)          Binary I/O stream class library @value{VERSION}
+* libbinio: (libbinio).         Binary I/O stream class library @value{VERSION}
 @end direntry

 @titlepage

2. The info files are developer docs, so they should better be packaged into the
*-devel package.

3. The pkgconfig file is non-functional:
# pkg-config --libs libbinio
Unknown keyword 'URL' in '/usr/lib/pkgconfig/libbinio.pc'


Non blocking issues:

* IMO, this is questionable design:
# pkg-config --cflags libbinio
-I/usr/include/libbinio

Is this really what upstream wants?

* Has this code been tested on big endian targets?
Comment 21 Linus Walleij 2005-09-27 09:36:40 EDT
Well, yeah sorry Ralf, patience is not my strong side. Curiously
enough it worked this time ;-)

I had this idea that once I have been submitted my first error-free 
package I would take on reviewing packages myself so more things get 
done. I know you're all volunteers. I try to be polite but whenever I 
fail feel free to correct me (as you did).

Anyway, I filed bugs upstream with your comments for (1) and (3) so let's 
hope it gets fixed.
http://sourceforge.net/tracker/index.php?func=detail&aid=1305865&group_id=69728&atid=525584
http://sourceforge.net/tracker/index.php?func=detail&aid=1305853&group_id=69728&atid=525584

The (2) issue is fixed but I'll wait for upstream and 
(hopefully) fix them all in next package.
Comment 22 Linus Walleij 2005-09-28 10:05:49 EDT
Comment on (3) from upstream:

>Comment By: Simon Peter (dynamite)
Date: 2005-09-28 12:54

Message:
Logged In: YES
user_id=31101

The pkgconfig file works here:
# pkg-config --libs libbinio
-L/home/simon/lib -lbinio

I'm using version 0.19 of pkg-config. I got the URL keyword
directly from the example in the manpage of pkg-config.

For the second point, I made cflags point to
/usr/include/libbinio for backwards compatibility. If i
would make it point to /usr/include and use #include
<libbinio/binio.h> instead, i would have to change all my
other software for the new include path. I don't know if
this is what Ralf meant.
Comment 23 Ralf Corsepius 2005-09-28 22:00:30 EDT
(In reply to comment #22)

> The pkgconfig file works here:
> # pkg-config --libs libbinio
> -L/home/simon/lib -lbinio
> 
> I'm using version 0.19 of pkg-config.
FC4 ships pkgconfig-0.15.0-6

=> Either upstream should not use "URL:"
or Linus should patch libbinio.pc
or libbinio-devel should Require: pkgconfig >= 0.19 (I.e. the package could not
be shipped for FC4)

> For the second point, I made cflags point to
> /usr/include/libbinio for backwards compatibility. If i
> would make it point to /usr/include and use #include
> <libbinio/binio.h> instead, i would have to change all my
> other software for the new include path. I don't know if
> this is what Ralf meant.
Yes, this is one point, I was referring to, but this is your (upstream's) decision.
Comment 24 Linus Walleij 2005-09-29 06:46:24 EDT
I will make a patch to remove the URL directive and once imported 
into the Extras I will consider reenabling the URL: directive for 
the devel version if and when it ships with a sufficiently new 
pkg-config. As for now it will be turned off.

A new package with all fixes is due soon.
Comment 25 Linus Walleij 2005-10-01 16:58:55 EDT
Here is a fixed spec and SRPM package:

Spec Name or Url: http://www.df.lth.se/~triad/krad/fc/libbinio.spec
SRPM Name or Url: http://www.df.lth.se/~triad/krad/fc/libbinio-1.4-3.src.rpm

(1) Texinfo file patched until upstream is fixed
(2) Texinfo files moved to devel package, along with the texinfo scriptlets
(3) Pkgconfig-file is patched to not used URL token for the time being
Comment 26 Ralf Corsepius 2005-10-06 12:36:39 EDT
You are patching the *.texinfo. Therefore you should 
BR: makeinfo
otherwise the *infos will not be regenerated.
Comment 27 Linus Walleij 2005-10-06 13:19:14 EDT
OK Ralf sorry for that real obvious thing. Here is a fixed package:

Spec Name or Url: http://www.df.lth.se/~triad/krad/fc/libbinio.spec
SRPM Name or Url: http://www.df.lth.se/~triad/krad/fc/libbinio-1.4-4.src.rpm
Comment 28 Ralf Corsepius 2005-10-07 13:47:46 EDT
I am still having doubts if this package works on big-endian targets, but the
packaging seems to be fine, now.

APPROVED
Comment 29 Linus Walleij 2005-10-11 14:51:26 EDT
OK imported and builds. Thanks Ralf, and the others.

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