alandefreitas / matplotplusplus

Matplot++: A C++ Graphics Library for Data Visualization 📊🗾
https://alandefreitas.github.io/matplotplusplus/
MIT License
4.11k stars 313 forks source link

CMake options missing project specific prefix #316

Closed globberwops closed 1 year ago

globberwops commented 1 year ago

Fixes #315

Add prefix "Matplot++_" to the following CMake options:

Before After
BUILD_EXAMPLES Matplot++_BUILD_EXAMPLES
BUILD_TESTS Matplot++_BUILD_TESTS
BUILD_INSTALLER Matplot++_BUILD_INSTALLER
BUILD_PACKAGE Matplot++_BUILD_PACKAGE
BUILD_WITH_PEDANTIC_WARNINGS Matplot++_BUILD_WITH_PEDANTIC_WARNINGS
BUILD_WITH_SANITIZERS Matplot++_BUILD_WITH_SANITIZERS
BUILD_WITH_MSVC_HACKS Matplot++_BUILD_WITH_MSVC_HACKS
BUILD_WITH_UTF8 Matplot++_BUILD_WITH_UTF8
BUILD_WITH_EXCEPTIONS Matplot++_BUILD_WITH_EXCEPTIONS
BUILD_HIGH_RESOLUTION_WORLD_MAP Matplot++_BUILD_HIGH_RESOLUTION_WORLD_MAP
BUILD_FOR_DOCUMENTATION_IMAGES Matplot++_BUILD_FOR_DOCUMENTATION_IMAGES
BUILD_EXPERIMENTAL_OPENGL_BACKEND Matplot++_BUILD_EXPERIMENTAL_OPENGL_BACKEND
WITH_SYSTEM_CIMG Matplot++_WITH_SYSTEM_CIMG
WITH_SYSTEM_NODESOUP Matplot++_WITH_SYSTEM_NODESOUP
alandefreitas commented 1 year ago

Nice. I think most projects (or all projects I've seen) just use uppercase for everything (MATPLOT++_BUILD_EXAMPLES). I've never seen options with special chars either but I'm not sure what the best replacement would be. MATPLOTPP_BUILD_EXAMPLES, MATPLOTXX_BUILD_EXAMPLES, MATPLOT_BUILD_EXAMPLES?

globberwops commented 1 year ago

@alandefreitas You're right. Browsing through awesome-cpp, the most common theme seems to be uppercase w/o special chars. So, MATPLOTPP_?

This line for handling BUILD_SHARED_LIBS makes a lot of sense to me, too, while we're at it: YAML_BUILD_SHARED_LIBS.

alandefreitas commented 1 year ago

So, MATPLOTPP_?

I think so. I'm not sure PP is the best suffix but it's better than before and we're breaking things anyway. Feel free to suggest anything else though.

This line for handling BUILD_SHARED_LIBS makes a lot of sense to me, too, while we're at it: YAML_BUILD_SHARED_LIBS.

Yes. Having a separate variable for the library while still using the default is a good idea.

WildRackoon commented 1 year ago

This line for handling BUILD_SHARED_LIBS makes a lot of sense to me, too, while we're at it: YAML_BUILD_SHARED_LIBS.

This design choice is fine, but note that usually the more CMake-way to do this involves using BUILD_SHARED_LIBS as-is, so that the command add_library() with no type specified (STATIC, SHARED, MODULE) can be deduced from BUILD_SHARED_LIBS usually given through the command-line. (See Documentation link)

Some library also chooses to always supply both a MATPLOTPP and a MATPLOTPP_static to be used explicitely rather than having to statefully disable/enable BUILD_SHARED_LIBS before/after using add_directory().

alandefreitas commented 1 year ago

This design choice is fine, but note that usually the more CMake-way to do this involves using BUILD_SHARED_LIBS as-is, so that the command add_library() with no type specified (STATIC, SHARED, MODULE) can be deduced from BUILD_SHARED_LIBS usually given through the command-line. (See Documentation link)

Yes. It should be fine because MATPLOTPP_BUILD_SHARED_LIBS will always match BUILD_SHARED_LIBS unless the user wants to mess with that.

Some library also chooses to always supply both a MATPLOTPP and a MATPLOTPP_static to be used explicitely rather than having to statefully disable/enable BUILD_SHARED_LIBS before/after using add_directory().

Yes. That I don't like. It's not honoring the BUILD_SHARED_LIBS contract and I've seen it cause some trouble for people packing the library (vcpkg in particular).

alandefreitas commented 1 year ago

I think MinGW, at least the way it's being fetched, has been broken for a while. The MinGW failure is just a failure in CI, right?

WildRackoon commented 1 year ago

I think MinGW, at least the way it's being fetched, has been broken for a while. The MinGW failure is just a failure in CI, right?

Maybe the C and CXX COMPILER are unsufficient, the canonical way is generally to add mingw64/bin to env paths and to solely use -G MinGW Makefiles.

Your github action image does not add it (through msys2) to path by default (See Readme: https://github.com/actions/runner-images/blob/win19/20221214.4/images/win/Windows2019-Readme.md#notes )

You can probably check this to see how to do it: https://github.com/actions/runner-images/blob/3b5c4ebd39ce3d5812e130938bb066f67d90b54e/images/win/scripts/Installers/Install-Msys2.ps1#L76

alandefreitas commented 1 year ago