cmark-gfm: add

[hdoc]

Per @eli-schwartz's suggestion in hdoc/hdoc#25, we'd like to contribute a Meson wrap for the cmark-gfm package.

This is a C library that implements parsing of Markdown files, and includes some extensions that GitHub (the company) has made. It's built using CMake, and this PR adds Meson support as a wrap.

[eli-schwartz]

Unfortunately, the upstream CMake config uses the generate_export_config CMake functionality (used here, here, and here) that doesn't seem to be supported by Meson. These lines generate the cmark-gfm-extensions_export.h and cmark-gfm_export.h files. Since I couldn't find a way to get Meson to generate these files, I've (temporarily) included them with the patch.

Because CMake is so amazingly wonderful, it generates a statically coded boilerplate file that really is trivial to check into git. It's pretty annoying that projects rely on a weird CMake function instead.

But the really bad part of this is that CMake generates a different file depending on your OS. This should just include support for all OSes via compiler check macros. Basically:

• if compiler supports GNUC 4 or greater, set up attribute visibility, it's never wrong to unconditionally mark visibility on supported compilers
• if compiler has the _MSC_VER define, check 8 or 10 different conditions before deciding whether to set up dllimport, dllexport, or nothing
• if all else fails, define macro as empty so it's a no-op

The CMake generated file just ritualizes this with project-specified scope names, including a bit of gunk nobody actually uses in their CMake projects.

[hdoc]

I agree that the CMake export functionality is extremely annoying. If we restrict this wrap to solely build cmark-gfm as a static library then we can drastically simplify the export files but ignoring all of the visibility #defines. That would leave the "deprecated" #defines. For those, I suggest we manually check if the compiler supports the deprecated attribute with some code like this:

cc = meson.get_compiler ('c')
if (cc.has_function_attribute('deprecated'))
  # Set the CMARK_GFM_DEPRECATED define to whatever
else
  # Don't set any defines
endif

That would then get written to the *_export.h files which would be generated at configure-time, perhaps using a generator.

Does this sound reasonable?

[eli-schwartz]

That would leave the "deprecated" #defines. For those, I suggest we manually check if the compiler supports the deprecated attribute with some code like this:

That's just random goop which CMake creates, and cmark-gfm doesn't use.

Anyway, I went and ported cmark to use a simpler header that Meson can use too. It would need to be merged into cmark, then merged again into cmark-gfm, then released, before this wrap could take advantage of it... but this wrap could still include the header itself in the meantime.

[eli-schwartz]

Here's a much better template to use.

$ cat src/cmark-gfm_export.h.in 
#ifndef CMARK_GFM_EXPORT_H
#define CMARK_GFM_EXPORT_H

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

/*
 * Here is the complicated part. Windows is special -- you cannot just define
 * entry points unconditionally.
 * */
#if defined _WIN32 || defined __CYGWIN__
   /* When building static libraries, avoid marking public ones */
#  if defined CMARK_GFM_STATIC_DEFINE
#    define CMARK_GFM_EXPORT
   /* We are building this library */
#  elif defined libcmark_gfm_EXPORTS
#    define CMARK_GFM_EXPORT __declspec(dllexport)
   /* We are using this library */
#  else
#    define CMARK_GFM_EXPORT __declspec(dllimport)
#  endif

/* On to the easy part. GCC and lookalikes such as clang just work */
#elif defined __GNUC__ && __GNUC__ >= 4
#  define CMARK_GFM_EXPORT __attribute__((visibility("default")))

/* Older solaris support, why not */
#elif defined __SUNPRO_C && __SUNPRO_C >= 0x550
#  define CMARK_GFM_EXPORT __global

/* All else failed, and we don't know about this compiler. Be conservative. */
#else
#  define CMARK_GFM_EXPORT
#endif

#endif /* CMARK_GFM_EXPORT_H */

I also removed the pointless duplication of headers... but that needs patches to cmark-gfm.

[hdoc]

Nice, thanks for making the PR upstream. Let me know if there is anything I can do to help. In the meantime I suppose we will let this PR sit on ice until upstream gets fixed, unless you have a better idea...

Including the headers will cause the PR to fail tests unfortunately so that's not an option, unless we want to special-case it.

[eli-schwartz]

Simply name the headers *.h.meson instead of *.h.in. :)

[eli-schwartz]

These commits should all be squashed into one commit, probably...

[hdoc]

@eli-schwartz thanks, I implemented your suggestion.

Commits are all squashed and the wrap builds and passes sanity check locally. I think the remaining items are to run CI on this to ensure it's clean, and then merge if everyone find it satisfactory.

[hdoc]

Looks like I made a mistake when I formatted the file, one moment while I fix it...

[hdoc]

Should be good to go now.

[neheb]

cl : Command line error D8021 : invalid numeric argument '/Wno-unused-parameter'

[hdoc]

Saw that, fixing it now.

[hdoc]

@neheb fixed, though only for compilers that support -Wno-unused-parameter. Not sure how to make it work on MSVC, we don't have much experience with it.

[eli-schwartz]

if (expression) is equivalent to if (expression), the grouping isn't doing anything here.

Also note there is:

add_project_arguments(cc.get_supported_arguments('-Wno-unused-parameter'), language: 'c')

which means you don't need to repeat the argument name.

[hdoc]

Thanks, the extra brackets were an honest mistake. Incorporated your feedback on the latest commit.

[neheb]

since there's dllexport functionality now, probably makes sense to use library() instead of static_library().

[eli-schwartz]

This assumes that we use static_library() down below, but I think we can safely use library() instead. It's possible to change this with default_options: ['default_library=static'] either in the project() function or when invoking dependency() from a parent project. In both cases, the user still has the ability to manually override that on the command line if they really want.

[hdoc]

Is there a way to check if the library is being built as a static library or a dynamic library? The reason I ask is because we have the section below hardcoded, but this should be dynamic depending on if the user wants it built statically or dynamically.

edata = configuration_data(
    {
        'CMARK_GFM_STATIC_DEFINE': true,
    },
)

If we want to use library() instead of static_library(), we'll have to set cdata.CMARK_GFM_STATIC_DEFINE at configure-time and I don't know to get that information needed to make that decision. Any pointers are appreciated.

[hdoc]

Never mind, I figured it out. Fixing it now.

[eli-schwartz]

Could be rebased again. :)

[hdoc]

Rebased it and incorporated all feedback so far.

[eli-schwartz]

On MSVC:

cmark-gfm| Checking if "has __builtin_expect" compiles: YES
cmark-gfm| Checking if "has __attribute__" compiles: NO

Apparently a compile test is NOT good enough for that one. :) Makes sense in retrospect, it's not syntactically wrong.

[hdoc]

Thoughts on these Windows linker warnings?

[neheb]

This should be squashed.

As far as the file, you could probably avoid all that mess by manipulating cpp_args. See my recent PRs doing just that.

[eli-schwartz]

You cannot manipulate cpp_args in order to solve the issue of the upstream main header attempts to #include a file that cmake generates containing the defines.

Also in general command line length bloat can cause bugs where e.g. Windows just errors out because too long, but sometimes it is regrettably unavoidable...

[neheb]

Sure you can. Just empty it out. That's what I did for libebml.

[eli-schwartz]

Why painfully bend over backwards to install an empty file, when if you are just going to hardcode a list of static defines you can just dump them as a set of configuration_data() values into the exact same configure_file()?

EDIT: can we please revert the libebml thing? It looks really awkward and painful.

[neheb]

I’d rather not. Note that it’s dllexport when building the library and dllimport when using it.

edit: do note it's also a bugfix for having one library (both actually) being forced to be built as static.

[hdoc]

I've squashed the series of commits just now.

I would also prefer to use the same semantics as upstream does by generating the full *_export.h files instead of dumping an empty file and using cpp_args.

I believe the last outstanding issue is the Windows linker warnings. I do not have a Windows machine to test with (other than CI), so I would appreciate any help (a patch is especially welcome) in solving that issue.

Thank you.

Edit: I just re-reviewed the code and I believe the latest commit should fix the Windows linker issues. Waiting for a CI build to confirm.

[hdoc]

Linker warnings on Windows are fixed. I'm squashing the PR and I think it is ready to be merged in now barring any final review comments.

[jpakkane]

Issues seem to be fixed and tests are green so merging. Further minor issues can be fixed in subsequent MRs.

[eli-schwartz]

Yup, agreed.

[hdoc]

Thank you for merging this PR in, and thank you @eli-schwartz and @neheb for your help in getting it ready to merge 🙂