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

build: remove CMARK_STATIC and CMARK_SHARED options #513

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

compnerd
Copy link
Contributor

@compnerd compnerd commented Jan 8, 2024

Remove the CMARK_STATIC and CMARK_SHARED options as one of the two must be enabled always as the cmark executable depends on the library. Instead of having a custom flag to discern between the library type, use the native CMake option BUILD_SHARED_LIBS allowing the user to control which library to build [1]. This matches recommendations to only build a single copy of the library [2].

[1] https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html
[2] https://alexreinking.com/blog/building-a-dual-shared-and-static-library-with-cmake.html

@compnerd compnerd force-pushed the single branch 4 times, most recently from ba7f60e to bf63752 Compare January 9, 2024 18:13
Remove the `CMARK_STATIC` and `CMARK_SHARED` options as one of the two
must be enabled always as the cmark executable depends on the library.
Instead of having a custom flag to discern between the library type, use
the native CMake option `BUILD_SHARED_LIBS` allowing the user to control
which library to build [1]. This matches recommendations to only build a
single copy of the library [2].

[1] https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html
[2] https://alexreinking.com/blog/building-a-dual-shared-and-static-library-with-cmake.html
@jgm jgm merged commit 853cf7c into commonmark:master Jan 9, 2024
12 checks passed
@compnerd compnerd deleted the single branch January 9, 2024 20:17
@nwellnhof
Copy link
Contributor

Note that this is a breaking change which should be documented in the release notes.

I'd also suggest to add an option setting the default to "shared":

option(BUILD_SHARED_LIBS "Build using shared libraries" ON)

@compnerd
Copy link
Contributor Author

Sure, happy to add a release note for this. I don't really have an opinion on the default. I'm personally a fan of dynamic linking (that is what Linux distributions tend to prefer), though I felt like the project tended to prefer static linking. I can change the default to shared if that is preferred. CC: @jgm

@jgm
Copy link
Member

jgm commented Jan 16, 2024

It's probably best if the default stays whatever it was before (static or shared, I don't even remember).

@compnerd
Copy link
Contributor Author

The tests seem to indicate BUILD_SHARED=YES generally, so I think that the expectation was static linking. I don't honestly know, but @nwellnhof seems to prefer dynamic, so we could do that?

@jgm
Copy link
Member

jgm commented Jan 16, 2024

I don't have strong feelings about this and happy to defer to the two of you.

@nwellnhof
Copy link
Contributor

The old default was to build both static and shared libraries. I agree that it's cleaner and less error-prone to only allow building one type of library. Regarding what should be built by default, I'd prefer shared libraries but that's just my opinion.

I think we should also add a bit of backward compatibility to not break the build for hundreds of users. See Github search results for example:

@jgm
Copy link
Member

jgm commented Jan 16, 2024

+1 on backwards-compatibility.

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

Successfully merging this pull request may close these issues.

3 participants