Skip to content

Better support for modern CMake #1374

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

Closed
wants to merge 19 commits into from
Closed

Better support for modern CMake #1374

wants to merge 19 commits into from

Conversation

Andres6936
Copy link

@Andres6936 Andres6936 commented Dec 26, 2021

CMake support has been improved to allow downloading the project with Fetch, configuration and linking is done automatically for a better development experience, the compilation mode is dependent on the option used and you can only compile SHARED or STATIC but not both at the same time.

@hjmjohnson
Copy link
Contributor

@Andres6936 all these small changes seem to be conceptually related. Reviewing the changes is difficult because some early changes are updated in later changes. I would recommend squashing these changes into self-contained patch sets that directly address the desired changes without introducing intermediary temporary steps. Perhaps squash all of this into 1 commit, or squash the "lower-case" fixups into the commits that introduced the uppercase cmake directives so that the diffs are smaller and easier to understand.

@Andres6936
Copy link
Author

I will try Sir, although to be frank I don't know how to do it without redoing a PR of everything already done.

endif()
# CMake link shared library on Windows
# Ref: https://stackoverflow.com/a/41618677
set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS TRUE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to make it an option rather than set forcefully

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact it is the only option that exists to obtain a similar behavior in both Linux and Windows without having to touch code or use the export directives in each symbol to be exported, this simulates the default behavior that occurs in Linux to export all symbols by default, which does not happen in Windows.

Although you are right, the code already has the JSON_API directives that solve this problem, I will add the option.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, option() with ON as default would allow user overwrite in case there's any compatibility issue with their setup.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion we should not add an eventual fix to a hypothetical event, for my part it is more convenient to wait for the confirmation of the compatibility problem with the incident report.

Likewise, if you have an article where the compatibility issue with CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS is discussed, I would like to have a document about them to start mitigating the impact of the change.

@Andres6936 Andres6936 requested a review from hjmjohnson January 17, 2022 21:38
This pull request was closed.
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