Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PACKAGE_* defines in io_lib_config.h #25

Closed
gt1 opened this issue Mar 16, 2020 · 7 comments
Closed

PACKAGE_* defines in io_lib_config.h #25

gt1 opened this issue Mar 16, 2020 · 7 comments

Comments

@gt1
Copy link
Contributor

gt1 commented Mar 16, 2020

This is more of a wish list item than an actual issue:

The current version of io_lib installs a config.h type header containing define statements for names such as PACKAGE_NAME and PACKAGE_VERSION. As this header is included by other library headers, this will lead to clashes with other packages using autoconf. It would be better not to have these defines in the installed header.

@jkbonfield
Copy link
Owner

I'm wondering whether installing io_lib_config.h was an accident. Infact I'm certain it probably was. I think it's meant to be internal to the build system as it includes other things which wouldn't necessarily be appropriate either, such as HAVE_LIBBZ2 and possibly FILE_OFFSET_BITS, etc.

There is some advice on this https://stackoverflow.com/questions/19586211/correct-installation-of-config-h-for-shared-library-using-autotools which basically states it's not a good idea. I agree with this.

There are a couple headers which have:

#ifdef HAVE_CONFIG_H
#include "io_lib_config.h"
#endif

That should probably go. If there are things I need in the header files that require configuration from autoconf, it ought to be done via a separate .in file which cherry picks the specific things, much like the @DEF@ bits already in io_lib/os.h.in.

I think I need to do some testing to see what breaks if I completely remove a public config.h.

That said, I assume you're looking at this because you want a central place to identify the io_lib version number? That should be put somewhere public, probably by creating an io_lib/version.h file with string and numeric forms of version number.

@gt1
Copy link
Contributor Author

gt1 commented Mar 16, 2020

I noticed the name clash when I tried to put the CRAM encoding interface for libmaus2 into a separate header file in io_lib and had an include for io_lib_config.h in the header for that interface (it turned out I did not need that, so I just removed it, but other packages may see the name clash when including io_lib/scram.h).

As for the io_lib version: I extract that information from the output of io_lib-config --version . But I guess it could be useful to have the version number available in a header as well.

jkbonfield added a commit that referenced this issue Mar 23, 2020
The header has macros for the string version (IOLIB_VERSION) along
with major, minor and patch version numerics. This replaces the need
to use io_lib_config.h which should never have been installed.

In order to validate this, I tested a build of progs directory using

    make DEFAULT_INCLUDES="-I/tmp/inst/include" \
         CPPFLAGS="-UHAVE_CONFIG_H" AM_CPPFLAGS=""

Although not strictly necessary as progs is part of the core package,
I wish it to be a demonstration of how to use the library without
requiring internal knowledge of the sibling io_lib directory.

This required a few extra HAVE_CONFIG_H guards along with the obvious
renaming of PACKAGE_VERSION to IOLIB_VERSION in several places.

Fixes #25
@jkbonfield
Copy link
Owner

Please take a look at branch https://github.com/jkbonfield/io_lib/tree/package_version and let me know your thoughts.

It seems to work for me, but I'd appreciate your testing too before merging.
Thanks.

jkbonfield added a commit that referenced this issue Mar 23, 2020
The header has macros for the string version (IOLIB_VERSION) along
with major, minor and patch version numerics. This replaces the need
to use io_lib_config.h which should never have been installed.

In order to validate this, I tested a build of progs directory using

    make DEFAULT_INCLUDES="-I/tmp/inst/include" \
         CPPFLAGS="-UHAVE_CONFIG_H" AM_CPPFLAGS=""

Although not strictly necessary as progs is part of the core package,
I wish it to be a demonstration of how to use the library without
requiring internal knowledge of the sibling io_lib directory.

This required a few extra HAVE_CONFIG_H guards along with the obvious
renaming of PACKAGE_VERSION to IOLIB_VERSION in several places.

Fixes #25
jkbonfield added a commit that referenced this issue Mar 23, 2020
The header has macros for the string version (IOLIB_VERSION) along
with major, minor and patch version numerics. This replaces the need
to use io_lib_config.h which should never have been installed.

In order to validate this, I tested a build of progs directory using

    make DEFAULT_INCLUDES="-I/tmp/inst/include" \
         CPPFLAGS="-UHAVE_CONFIG_H" AM_CPPFLAGS=""

Although not strictly necessary as progs is part of the core package,
I wish it to be a demonstration of how to use the library without
requiring internal knowledge of the sibling io_lib directory.

This required a few extra HAVE_CONFIG_H guards along with the obvious
renaming of PACKAGE_VERSION to IOLIB_VERSION in several places.

Fixes #25
@gt1
Copy link
Contributor Author

gt1 commented Mar 24, 2020

Apologies for the late reply. This current fails for me. I include io_lib/scram.h in an autoconf project, which defines HAVE_CONFIG_H. This includes "io_lib_config.h", which does no longer exists in the installation.

@jkbonfield
Copy link
Owner

Hmm. I think I should make sure all public header files don't have the HAVE_CONFIG_H check in them too and reserve it purely for the C files. I'll work on that.

Thanks for testing.

jkbonfield added a commit that referenced this issue Mar 24, 2020
Removes the #ifdef HAVE_CONFIG_H from a couple more public header
files, and ensures all C files include it.
@jkbonfield
Copy link
Owner

I'm hoping master now has this fixed. Sorry for the woes and thanks for reporting the issue.

@gt1
Copy link
Contributor Author

gt1 commented Mar 25, 2020

Yes, it works for me now. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants