[BuildTools] distributed configure headers trouble

[Matěj Týč]

On Sat, 2010-05-15 at 23:38 -0400, Ted Ralphs wrote:
> Matěj, thanks for looking into this. I actually spent a bit of time
> already looking into it myself, so let me just say what I've found so
> far and clear up a little confusion. First, it's not too obvious how
> the whole thing hangs together at the moment, so let me summarize:

Thank you for your accurate reply.

> 1. Each project XXX has a header file called XXXConfig.h, which is
> hand-generated and is used in one of two modes.
> 
> --If HAS_CONFIG_H is defined, then the header config_xxx.h is included
> and all the potentially clashing macros are undefined to avoid
> problems if the config headers of other projects are also included. At
> the moment, HAS_CONFIG_H is defined automatically for all projects
> during an autotools build, so that including XXXConfig.h gives access
> to all the everything defined in config_xxx.h. For most projects, the
> files XXXConfig.h and config_xxx.h are both installed. Furthermore,
> the latter is installed with the following prepended to it:
> #ifndef HAS_CONFIG_H
> #define HAS_CONFIG_H
> #endif
> The effect is that if a user later include the installed version of
> XXXConfig.h in his own code, she *must* also have config_xxx.h
> available or the build will break.

An immediate improvement of the situation as I understand so far would
be to wrap the config stuff into an #ifdef block (like #ifdef
COIN_CONFIG), so it would be visible only during Coin stuff
configuration and build (a -DCOIN_CONFIG CPP flag would have to be
passed to the compiler).
Just out of curiosity, what do you think about this idea (I am asking in
order to identify possible misconceptions).
However I think that it will be better to wait and remove those headers
completely from the installation. I think that it is possible, see below

> 
> --If HAS_CONFIG_H is not defined, then the hand-generated header
> configall_system.h is included. RIght now, this never occurs if the
> autotools are used. The purpose of this header file is to define
> certain required variables on platforms where the autotools are not
> available. At the moment, the only platform supported in this mode is
> Visual C++. If the detected compiler is cl, then the file
> configall_system_msc.h is included. These latter two files live in
> BuildTools/headers and are pulled in by the project files in MSVC++
> only. They are otherwise not needed at the moment.

Oh yes, I see now. This autotools emulation is looks fine.

> 
> 2. I believe the main purpose of making config_xxx.h available to
> another project YYY is in case the details of how XXX was built
> affects how YYY should be built. For many of the high-level projects,
> there probably won't be any external dependence on config_xxx.h.
> However, for some lower-level libraries, there is perhaps more
> dependence than would be expected for separate libaries. For example,
> Clp depends strongly on the factorization routines in CoinUtils and I
> can imagine it makes a substantial difference whether BLAS and LAPCK
> are present or not when building Clp, but this is part of the
> configuration of CoinUtils. Furthermore, I have just added the
> capability to automatically generate version numbers for all projects
> and put them in config_xxx.h. If one projects wants version info about
> another, then the current set-up is that it would get it through
> config_xxx.h (though we could change that).

What comes in my mind is that it would be good if the fact like "I have
CoinUtils installed" would mean one thing in terms of API. Have I
understood correctly that it is not the case ATM, because if you haven't
BLAS or LAPACK available during build time, certain API functions are
completely missing? Or maybe they are not missing, but certain
functionality is missing? Or is it just a speed issue?

I have encountered a somewhat similar issue as a developer of an image
loading library, that depends on various other libraries (like libpng,
libtiff, ...) in order to handle certain formats. There is one function
for loading images, and if you would like to load an image that requires
a library you don't have, you will discover that at runtime when a load
fails (= the public API does not depend on anything that may or may not
be used).
If the API itself depends on some external stuff, I would suggest
splitting the library into the "minimal trunk" and various "addons".
Like that a user knows exactly what he has installed and nothing like "I
have Clp, he has Clp, but it works for him but not for me" can happen...

> 
> So the bottom line is that with the current set-up, if we are building
> with the autotools, we may need access to both XXXConfig.h and
> config_xxx.h when building YYY. For each project, both of these
> configuration files are available to all other projects during the
> build process if all projects are built simultaneously, as with the
> current build system. However, this might no longer be the case under
> the new build system if we were to disable installation of
> config_xxx.h. At present, this would break the build process for
> almost all projects under that system. Basically, any code in YYY that
> includes a file XXXConfig.h would be broken. And it's not just the
> libraries themselves we need to worry about---some of the examples may
> also include these configuration files. In some cases, I think the
> configuration file may even be included in another header that
> contains parts of the API. In all likelihood, these inclusions are
> unnecessary, but we still need to be aware of them.

I think that the configure script should be able to test for every
relevant information that can be transmitted through that header anyway.
Gnulib allows reliable checking that a certain function or class is
present, and although it is not always feasible to check a particular
behavior of a function / method, I think that this should be good
enough.
The truth is that the configure script can't easily check for version
numbers. On the other hand, what the autotools guys say quite often is
"check for features, not for version numbers". Hmmm...

> 
> It seems to me that there are several solutions to this problem, but
> the first step is to figure out what configuration variables really
> need to be exposed. Once we know what they are, the easiest solution
> seems to be to put them directly into XXXConfig.h rather than into
> config_xxx.h. Then we install XXXConfig.h (and not config_xxx.h) and
> we cease prepending the lines that always define HAVE_CONFIG_H. Then
> if access to these variables is needed, we'll just include
> XXXConfig.h. Once config_xxx.h is not included by installed versions
> of XXXConfig.h, there shouldn't be any conflict.

Yes, I think that I understand. 

> 
> I hope some of that made sense. It's a bit convoluted and I didn't
> know myself exactly how it all hung together until I really started
> poking around. I am working through the projects as I have time and
> trying to see how easily we can disengage the requirement to install
> config_xxx.h. So far, I've succeeded with SYMPHONY and DIP without a
> lot of difficulty (thought it did take some time). I expect it will be
> quite easy also with some others, like Bcp, Cbc, Couenne, Bonmin,
> etc., but not so easy with CoinUtils, Cgl, Clp, etc.
> 
> You had said you would post an example of how to get access to the
> values of variables defined in config_xxx.h without explicitly
> including it. I can imagine several ways to do that. Can you say more
> about how you would suggest to do it?

I actually did that in the beginning of the previous mail, I am sorry if
it wasn't understandable enough :-)

The idea is that instead of getting a particular information (for
example a version string) via a header macro, you call a library
function that has been designed as a wrapper of that macro.

So in the example below, you can see that the macro PACKAGE_NAME has
been wrapped by the function const char * get_cbc_version(). This can be
slightly misleading because the PACKAGE_NAME macro actually doesn't
contain version information, but the key concept is "wrapping of macros
by functions".

Regards, 
Matej


> 
> Cheers,
> 
> Ted
> 
> On Thu, May 13, 2010 at 6:10 PM, Matěj Týč <matej.tyc at gmail.com> wrote:
> > 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
> >
> >
> > _______________________________________________
> > BuildTools mailing list
> > BuildTools at list.coin-or.org
> > http://list.coin-or.org/mailman/listinfo/buildtools
> >
> 
> 
>