autoconf config.h

[DasVinch]

So after making a hot mess with the autoconf system with #30 and https://github.com/milk-org/ImageStreamIO/pull/64.

"config.h" should never be #included from .h files, only from .c files -- it shouldn't be part of a public header (see here: https://stackoverflow.com/questions/7398/how-to-avoid-redefining-version-package-etc)

I mostly resolved that out of ImageStreamIO with https://github.com/milk-org/ImageStreamIO/commit/caeb39b5b4dc647d25c04aeef58f89a7d5a936e5 and https://github.com/milk-org/ImageStreamIO/commit/a70528c45e1be9dc90b3d2a2118530f097cc2484

Only one place in MILK actually needs the IMAGESTRUCT_VERSION from ISIO, in src/CommandLineInterface/CLIcore/CLIcore_help.c. I've fixed this at 1ee867c8d4028e728a94fcacdfb9da2efcdaa1a1 with a #include "ImageStreamIO/ImageStreamIO_config.h" (in a C file, not H).

Now the problem this causes is that CLIcore_help includes both MILK's config.h and ISIO's config.h, which causes symbol redefinition and thus spits the following warning:

[ 24%] Building C object src/CommandLineInterface/CMakeFiles/CLIcore.dir/CLIcore/CLIcore_help.c.o
In file included from /home/vdeo/src/milk/src/CommandLineInterface/CLIcore/CLIcore_help.c:16:
/home/vdeo/src/milk/src/COREMOD_arith/../ImageStreamIO/ImageStreamIO_config.h:1: warning: "PROJECT_NAME" redefined
    1 | #define PROJECT_NAME "ImageStreamIO"
      | 
In file included from /home/vdeo/src/milk/src/COREMOD_arith/../config.h:1,
                 from /home/vdeo/src/milk/src/COREMOD_arith/../CommandLineInterface/CLIcore.h:50,
                 from /home/vdeo/src/milk/src/CommandLineInterface/CLIcore/CLIcore_help.c:15:
/home/vdeo/src/milk/src/COREMOD_arith/../milk_config.h:1: note: this is the location of the previous definition
    1 | #define PROJECT_NAME "milk"
      | 
In file included from /home/vdeo/src/milk/src/CommandLineInterface/CLIcore/CLIcore_help.c:16:
/home/vdeo/src/milk/src/COREMOD_arith/../ImageStreamIO/ImageStreamIO_config.h:2: warning: "VERSION_MAJOR" redefined
    2 | #define VERSION_MAJOR 2
      | 
In file included from /home/vdeo/src/milk/src/COREMOD_arith/../config.h:1,
                 from /home/vdeo/src/milk/src/COREMOD_arith/../CommandLineInterface/CLIcore.h:50,
                 from /home/vdeo/src/milk/src/CommandLineInterface/CLIcore/CLIcore_help.c:15:
/home/vdeo/src/milk/src/COREMOD_arith/../milk_config.h:2: note: this is the location of the previous definition
    2 | #define VERSION_MAJOR 1
      | 
In file included from /home/vdeo/src/milk/src/CommandLineInterface/CLIcore/CLIcore_help.c:16:
/home/vdeo/src/milk/src/COREMOD_arith/../ImageStreamIO/ImageStreamIO_config.h:3: warning: "VERSION_MINOR" redefined
    3 | #define VERSION_MINOR 00
      | 
In file included from /home/vdeo/src/milk/src/COREMOD_arith/../config.h:1,
                 from /home/vdeo/src/milk/src/COREMOD_arith/../CommandLineInterface/CLIcore.h:50,
                 from /home/vdeo/src/milk/src/CommandLineInterface/CLIcore/CLIcore_help.c:15:
/home/vdeo/src/milk/src/COREMOD_arith/../milk_config.h:3: note: this is the location of the previous definition
    3 | #define VERSION_MINOR 03
      | 

Now the clean way to get rid of this would be to get rid of the #include "config.h" from CLIcore.h. This however break a macro INIT_MODULE_LIB and function RegisterModule that's defined directly in that h file and refers to Milk's version

To be continued... @stefi07 lmk if you have thoughts on how to be proper there.

[stefi07]

I wonder if we're trying to generalize too much here. I included PROJECT_NAME, VERSION_MAJOR etc. in ImageStream.h.in, but the only thing really needed is IMAGESTRUCT_VERSION, right? So maybe just have that in ImageStream.h.in and then nothing else gets redefined. I can push a quick update with this. What do you think, @DasVinch ?

[DasVinch]

the only thing really needed is IMAGESTRUCT_VERSION, right? In that case, correct! But you were also correct in making ImageStreamIO follow a standard format for the config.h.in contents, which in turn highlighted we were doing it wrong in MILK.

So far so good, since we compile and run. So no rush in fixing just yet I'd say.

But the same issue will arise if you try to include/link a 3rd party using milk as a dependency and using the same config.h autogen structure. Which is also what you were trying to address with the pkg_config standardization you PR'd?

[stefi07]

So far so good, since we compile and run.

Phew, that's great. I've now pulled the updated dev branches for both and I can confirm it.

But the same issue will arise if you try to include/link a 3rd party using milk as a dependency and using the same config.h autogen structure. Which is also what you were trying to address with the pkg_config standardization you PR'd?

I think the issue only arises in the case where one project is included as a subfolder / submodule of a different project, as is the case with ImageStreamIO and milk. In this case, they are both built together and during the build these variables keep getting redefined.

In the case of my PR I am including milk as a dependency in the CMakeLists.txt of my project (first finding it with find_package and then linking it via target_link_libraries), so I am not recompiling milk at the same time as the project, it just checks if it's installed and links it.

Another solution I can think of is to include the project name in the definitions for the overlapping variables. So instead of VERSION_MAJOR, have milk_VERSION_MAJOR and ImageStreamIO_VERSION_MAJOR in the respective config.h.in files. This is supported by the cmake documentation, since project() sets both PROJECT_VERSION_MAJOR and <PROJECT-NAME>_VERSION_MAJOR (and the same for all the other version-related vars). The only variable for which this solution wouldn't work is PROJECT_NAME, but I would argue that isn't a variable and shouldn't be in the config.h.in file anyway. What are your thoughts about this?

[DasVinch]

In the case of my PR I am including milk as a dependency in the CMakeLists.txt of my project (first finding it with find_package and then linking it via target_link_libraries), so I am not recompiling milk at the same time as the project, it just checks if it's installed and links it.

But that does mean that your project #include some milk header files at some point, right?

[stefi07]

Apologies for the slow reply, @DasVinch. Yes, but actually I believe that at this point I'm only including ImageStreamIO headers, so I'll likely encounter the same warnings in my project when I do include milk headers as well.

What were your thoughts on the explicit namespace separation (e.g. MILK_VERSION_MAJOR and IMAGESTREAMIO_VERSION_MAJOR in the respective config.h.in as in my previous reply)?

A variation on that would be to keep the milk_config.h variables as they are (PROJECT_NAME, VERSION_MAJOR etc.) and have a header guard for the ImageStreamIO_config.h such that namespaced variables are always available, and generic variables get defined if they haven’t been already (essentially accounting for ImageStreamIO being as a main project or as a submodule of milk). I’m thinking something along the lines:

// @file ImageStreamIO_config.h.in

#ifndef PROJECT_NAME

// Main project configuration
#define PROJECT_NAME "@PROJECT_NAME@"
#define VERSION_MAJOR @VERSION_MAJOR@
#define VERSION_MINOR @VERSION_MINOR@
#define VERSION_PATCH @VERSION_PATCH@
#define VERSION_OPTION "@VERSION_OPTION@"

#endif

// Submodule project configuration
#define IMAGESTREAMIO_PROJECT_NAME "@PROJECT_NAME@"
#define IMAGESTREAMIO_VERSION_MAJOR @VERSION_MAJOR@
#define IMAGESTREAMIO_VERSION_MINOR @VERSION_MINOR@
#define IMAGESTREAMIO_VERSION_PATCH @VERSION_PATCH@
#define IMAGESTREAMIO_VERSION_OPTION "@VERSION_OPTION@"

#define IMAGESTRUCT_VERSION "@VERSION_MAJOR@.@VERSION_MINOR@"

[DasVinch]

Oh that header guard is clever! In a hack way :) So I like it.

That's a good thing to keep in mind in case we hit a pkg_config snag.

[drbitboy]

It's been a while since I did anything with autoconf: is the @whatever@ is the substitution trick for autoconf?