use an in-tree header for symbol export information

[eli-schwartz]

Relying on CMake's GenerateExportHeader produces a file that is longer than the one being added here, which for all that is just mostly defining macros not used here. And after all that, it only contains macros specific to a single compiler, while failing to consistently handle GNUC (that always supports symbol visibility even for static libraries).

Replace this with a more targeted header that is easy to read or include into external build systems, and which is also more robust than the one that only exists inside CMake.

[eli-schwartz]

Found via mesonbuild/wrapdb#719

[jgm]

@nwellnhof what do you think?

[nwellnhof]

And after all that, it only contains macros specific to a single compiler, while failing to consistently handle GNUC (that always supports symbol visibility even for static libraries).

CMake handles a few more compilers besides gcc and I don't see why visibility should be set for static libraries. CMake doesn't seem to support Solaris, though. The more important point is that using GenerateExportHeader makes it impossible to use other build systems, so +1 to the change.

The libcmark_gfm_EXPORTS check should be changed to libcmark_EXPORTS, of course. It might also be a good idea to add a test that verifies that symbols are successfully hidden. Otherwise, mistakes like this can easily slip through. Such a test is highly platform-dependent, though. On Linux, one could test that the following command produces no output:

nm -g build/src/libcmark.so | grep cmark_parse_inlines

[nwellnhof]

I think we still support building both static and shared libraries, so the following check seems wrong:

/* Is this an exclusively static build */
#if @CMARK_STATIC_DEFINE@ && ! defined CMARK_STATIC_DEFINE
#  define CMARK_STATIC_DEFINE
#endif

This should probably be if ! @CMARK_SHARED_DEFINE@ .... Maybe we should simply remove the check which would make the header a static, non-generated file which could then be merged into cmark.h.

[jgm]

@nwellnhof I'll defer to your judgment on this. Do you want to suggest a specific change to this PR, or a new PR on top of it?

[eli-schwartz]

CMake handles a few more compilers besides gcc

What does "support" mean? What compilers are you referring to? CMake only supports two styles, that being GCC style and MSVC style, and it checks for some compilers that are too old or shouldn't support it before skipping a compiler test to see if the compiler supports it.

For context, here's what GenerateExportHeader does, and here's what my PR does:

• if building win32 or cygwin, use __declspec for dllexport/dllimport
• else, if compiler supports GNU-style visibility, use that
• This PR only: if certain versions of the Solaris compiler that do not support GNU-style visibility attributes, but have an exactly equivalent solaris-specific attribute, use the solaris attribute
• else, define macros to empty so they are a no-op

The implementation of these steps is a bit different:

• for win32 or cygwin:
  • GenerateExportHeader uses the CMake variables WIN32 or CYGWIN
  • my PR uses the compiler macros _WIN32 || __CYGWIN__
• for GNU-style visibility:
  • GenerateExportHeader checks again if not win32 or cygwin, then checks if gcc --version is at least 4.2, or if Intel is at least 12.0, or if the compiler is NOT XL, Watcom, or PGI, and if all these conditions are true, it then tries to run the compiler and see if -fvisibility=hidden is an accepted flag
  • my PR uses the compiler macro __GNUC__ which compilers such as clang define to indicate that they support GNU extensions to C

...

I am not positive but I think the __GNUC__ check should be equal... I am not even sure why CMake checks for most of this, because the commits implementing it are all 11 years old and I cannot find any references to whatever code review they used before gitlab. The Watcom check specifically does call out the fact that it's only excluded to "make the macros do almost nothing", so I think the main point of this all is to optimize out running the compiler in cases where it's known the accepted-flag check is going to fail.

and I don't see why visibility should be set for static libraries.

Yes, visibility should ideally be set for static libraries if the platform supports it. It allows the compiler to generate optimized code, and there are cases where you think you're building a static library but the user is, for example, planning to merge those static archives together into a fat library for some custom deployment (using link_whole).

I think we still support building both static and shared libraries, so the following check seems wrong:

The idea behind this check was originally that:

• when building shared only, none of these conditions are true, the only valid link is without static_define, and this happens by default
• when building both static and shared, the header is set up as with shared, so that using shared is by default
• when building static only, the first condition is true, and the only valid link is with static_define, and this happens by default.

So I think that the check is correct as is, although the comments below apply to it.

Maybe we should simply remove the check which would make the header a static, non-generated file which could then be merged into cmark.h.

Of course, indeed this whole block of code isn't needed. Other projects simply need to have static_define passed whenever linking to the static library, and this can be provided via pkg-config or *-config.cmake

If you'd prefer that I can make that change.

[eli-schwartz]

The libcmark_gfm_EXPORTS check should be changed to libcmark_EXPORTS, of course.

Yup, my bad, dumb typo when merging code across repos. Will fix it as soon as I'm sure of the best way to incorporate the other feedback.

[nwellnhof]

there are cases where you think you're building a static library but the user is, for example, planning to merge those static archives together into a fat library for some custom deployment

Fair enough.

So I think that the check is correct as is, although the comments below apply to it.

Right, if CMARK_SHARED is set, CMARK_STATIC_DEFINE will always be zero. I missed that.

By the way, if you really want to support Solaris' C compiler, you'll probably have to set a compiler option equivalent to -fvisibility=hidden. I think it's -xldscope=hidden.