commonmark / cmark

CommonMark parsing and rendering library and program in C
Other
1.63k stars 544 forks source link

build: provide some backwards compatibility for option changes #527

Closed compnerd closed 8 months ago

compnerd commented 8 months ago

Introduce an author warning for the use of CMARK_SHARED and CMARK_STATIC to redirect the author of the dependent package to BUILD_SHARED_LIBS.

compnerd commented 8 months ago

@nwellnhof, @jgm - okay, this is a first attempt to provide a migration path for the replacement of the custom options with standards. I think that we now return back to the original question of what should the default be? As written, this will give precedence to CMARK_STATIC over CMARK_SHARED. But if the default is BUILD_SHARED_LIBS=YES, perhaps we should consider giving higher precedence to CMARK_SHARED?

jgm commented 8 months ago

I think we should have it behave, by default, as it behaved in the past.

compnerd commented 8 months ago

AIUI, the original behaviour preferred the static linking of the binaries. So perhaps we should prefer static linking then. I don't think that we should provide the complete equality with providing the ability to build both variants of the library. If you really feel strongly about it, it is possible to do an internal secondary build of cmark that would allow you to have both libraries, but the idea is that you should choose whether you want a static library or a dynamic library.

jgm commented 8 months ago

Sorry, I no longer recall all the details. Are you saying that, previously, you could enable both CMARK_STATIC and CMARK_SHARED and it would build both, but that this is no longer possible with the current configuration?

compnerd commented 8 months ago

Yes, I think that it was feasible to generate both previously (which is why the build was duplicating rules and breaking the various recommendations for the modern use of CMake). The changes have now brought the CMake usage to the recommendations and adopted well-known CMake standard options over custom ones. While possible to do a recursive build and get the shared library and static library simultaneously, I think that it should be avoided and the user can select one of the two options.

jgm commented 8 months ago

Should we then have cmake raise an error with an explanation if they try to enable both CMARK_STATIC and CMARK_DYNAMIC? Or at least a warning saying that only the dynamic one will be built (or whatever the default is)?

jgm commented 8 months ago

@nwellnhof do you have an opinion about the default here?

compnerd commented 8 months ago

That is basically what I did with this change :)

  1. If the old CMARK_SHARED is set, we build a dynamic library (with a warning to say, use BUILD_SHARED_LIBS).
  2. If the old CMARK_STATIC is set, we build a static library (with a warning to say use BUILD_SHARED_LIBS).
  3. If both are set, we will emit the warning twice, and then build the static version (as it seems that the fuzzing assumes that the static library is available by default). This matches the default behaviour of CMake incidentally, which is actually good news IMO.