Skip to content

Conversation

@kring
Copy link

@kring kring commented Oct 29, 2025

  • Adds CXX_STANDARD=17 property to each of three CLI targets. Otherwise, compilation fails (at least on Windows / VS2022) when the non-C++-17 targets attempt to include headers that require C++17.
  • Wraps all of the CLI tools targets in a BUILD_TOOLS conditional. This can be turned off by those who only need the library.

I implemented both of these changes while adding the v2.1.0 release to vcpkg, but it would be better to have them upstream (here) if they're acceptable to the maintainers.

CC @hobu because this overlaps a bit with your #51.

@kring kring changed the title Add BUILD_TOOLS options, enable C++17 for CLI targets Add BUILD_TOOLS option, enable C++17 for CLI targets Oct 29, 2025
@kring
Copy link
Author

kring commented Oct 31, 2025

The reason the Windows build with BUILD_SHARED_LIBS=ON failed is because this library doesn't explicitly export any symbols, which is required for shared libraries on Windows. As a result, MSVC doesn't produce an export .lib, and the link of the CLI tools fails. In vcpkg, we disable spz for use as a shared library on Windows for this reason. The easiest (but not best) way to fix this is to add CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=ON to the command-line. I went ahead and did that in this PR just now. The better option (but much more effort) is to think about the public API and add all the necessary __declspec(dllexport) lines everywhere (usually via a macro).

The reason the Windows build with BUILD_SHARED_LIBS=OFF failed is because the other one failed, and the CI system is set up to cancel remaining builds after the first failure. If you want to change that behavior, add fail-fast: false to the test.yml file under jobs -> build -> strategy.

@hobu
Copy link
Contributor

hobu commented Oct 31, 2025

The easiest (but not best) way to fix this is to add CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=ON to the command-line.

We do this for the Conda Forge package because policy there is to not statically link.

I agree it would be good to clearly state the public API with decorated exports on both unix and MSVC. And fix the type initialization warnings. And add proper tests 😄

It's a bit frustrating to have merged the CI PR and then release it while it is ❌. This is why I asked if it was a stated goal of the project to have MSVC builds.

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.

2 participants