[BuildTools] distributed configure headers trouble

Matěj Týč matej.tyc at gmail.com
Thu May 13 18:10:21 EDT 2010


Hello all,
there is a bugreport in the tracker concerning installation and
inclusioin of private configure headers that contain macros that are
very likely to conflict with the user's macros.
The files in question are include files in include/coin directory that
contain 'config' string in their name.
I have grepped the /usr/include/coin directory and the only usage of the
PACKAGE_NAME macro was either defining or undefining it. So the fact
that if I remove the inclusion of config_..., and nothing happens (at
least not in the case of CBC and its dependencies) is not a big surprise
after all.

The only purpose those macros may serve are in moments like this:
// file my-file.cpp, the user's source code
#include <coin/cbc.h>

...

std::string cbc_version = PACKAGE_NAME;

This has two cavetas: 
 1. You might want to use that macro for your own package name
 2. This does not work ATM anyway since the config_... actually
#undefines that macro, so there is no benefit whatsoever.
However, this functionality can be accomplished even without having the
header with various defines installed on your machine:

// the COIN library source

#ifdef HAVE_CONFIG_H
#include "config-stuff.h"
#endif // HAVE_CONFIG_H

...

const char * get_cbc_version()
{	return PACKAGE_NAME;	}

... 

And you write this in your code

// file my-file-best.cpp, your source code
#include <coin/cbc-stuff.h>

...

// Now I can do the same as if I would have those macros defined!
std::string cbc_version = get_cbc_version();

There can be one "standard" function like Get<stuff>Version (where
<stuff> can be Cbc, Osi, Clp, ...) that would return versions, and
another for package names, used CXXFLAGS and LDFLAGS (during
compilation) etc.

However, there are other macros used out there, like COIN_HAS_CBC, that
IMO should not be there.
I suggest having all possible definitions defined and let the users
suffer if they use something they shouldn't since they don't have the
right library :-) But seriously, having #ifdef blocks all over the place
goes against code transparency and readability and warnings comming from
usage of undefined functions (= during linkt time) are quite
straightforward.

Next, there is an interesting line somewhere in the public headers:
#include "configall_system.h" There is no such file installed, it is
only used at build time.

Another artifact is the usage of the HAVE_CONFIG_H macro in public
headers. I don't think that it was intended to be in effect during the
usage of the library.

I would also suggest removing certain features.
For instance public headers contain lots of classes inline functions.
Although there may be a reason for this, I don't like it. Not only it
does contribute to plaguing the code with those COIN-related #ifdef
blocks, but it also introduces #ifdef blocks for 3rd party libraries,
like Readline, for instance.
I think that all readline usage should be covered in the library and
should not "leak" out if you know what I mean :-) I think that this may
be a pain for potential package maintainers of COIN projects.
Packages should not interfere with each other, like overwriting their
headers (I think that it is the case now)

There is quite a lot of information that is not structured so well,
therefore I sum it up here:
  - Not installing headers containing config or Config should be OK.
  - A function that would tell the user what version of a particular
Coin package is he using would be quite easy to use, but I don't see
anything like that implemented so far.
  - As many #ifdef blocks as possible should be removed because they
cause troubles when a user installs one Coin package and decides to
install another one later

That's it, thank you for your attention :-)
Regards,
Matej




More information about the BuildTools mailing list