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

Support C++20 module with optional standard library module. #61

Merged
merged 17 commits into from
May 15, 2024
Merged

Support C++20 module with optional standard library module. #61

merged 17 commits into from
May 15, 2024

Conversation

stripe2933
Copy link
Contributor

This PR adds C++20 module feature with optional standard library module (can be enabled via FASTGLTF_USE_STD_MODULE macro).

  • fastgltf.cppm module file exports symbols. Note that helper classes/functions from base64.hpp, util.hpp are not exported and therefore invisible to the end user.
  • Do not include standard library headers if FASTGLTF_USE_STD_MODULE macro defined, and use std module instead. This can dramatically reduce the compile time.

Copy link
Owner

@spnda spnda left a comment

Choose a reason for hiding this comment

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

How is FASTGLTF_USE_STD_MODULE expected to be defined? How does this work together with CMake? Can we add anything to the CI to test for these changes?

Also, this doesn't include the changes from #49.

Though thank you for this change, appreciate it!

module/fastgltf.cppm Outdated Show resolved Hide resolved
module/fastgltf.cppm Outdated Show resolved Hide resolved
module/fastgltf.cppm Outdated Show resolved Hide resolved
@stripe2933
Copy link
Contributor Author

stripe2933 commented May 11, 2024

FASTGLTF_USE_STD_MODULE can be defined as CMake target_compile_definitions command, like this.

I created the test repository with latest Clang CI (runs on ubuntu and macOS). For now, it has dependency of my another repository for using std module. But CMake ≥ 3.30 will support it natively, so I'm looking forward that the CI time would be reduced.

CMakeLists.txt Outdated Show resolved Hide resolved
module/fastgltf.cppm Outdated Show resolved Hide resolved
@spnda
Copy link
Owner

spnda commented May 12, 2024

FASTGLTF_USE_STD_MODULE can be defined as CMake target_compile_definitionscommand, like this.

Would it perhaps make sense to have this auto-defined when using a supported compiler? Or could that break things?

But CMake ≥ 3.30 will support it natively, so I'm looking forward that the CI time would be reduced.

I guess I'll just add CI runners for the modules later when that releases. Right now, I don't think I want to convoluted the CMake codebase for that.

@stripe2933
Copy link
Contributor Author

FASTGLTF_USE_STD_MODULE can be defined as CMake target_compile_definitionscommand, like this.

Would it perhaps make sense to have this auto-defined when using a supported compiler? Or could that break things?

The macro can be replaced with __cpp_lib_modules >= 202207L and it will not break anythings, but AFAIK no compilers implemented this feature testing macro yet (still experimental state). However, combining C++20 module feature and standard library header is very problematic, especially for MSVC. As mentioned in above, not using the conventional header is the best way for porting to module.

My suggestion is: Use FASTGLTF_USE_STD_MODULE and let user manually choose to use standard library module or not for now. After module is sufficiently stabilized, replace it to __cpp_lib_modules.

@spnda
Copy link
Owner

spnda commented May 12, 2024

I've tested my CMake changes locally and they seem to work just fine on Clang 18 and VS 17.9. It also compiles on GCC 14.1 but it takes forever to link. I've just added a warning for now about the import std stuff being used on pre-C++23. Is there anything I perhaps overlooked, since I don't know much about CMake and modules?

@stripe2933
Copy link
Contributor Author

stripe2933 commented May 13, 2024

Well, CI builds failed in my case. stripe2933/fastgltf-module-test@69a28f5

My CppStandardLibraryModule links std and std.compat targets to the all targets in the directories using link_libraries (which is independent to the destination target). It is conflicted with your fastgltf::module target, because:

  • If std target declaration is before the fastgltf::module target declaration, fastgltf::module cannot use std target unless it explicitly linked, e.g.
    target_link_libraries(fastgltf_module PUBLIC std std.compat)
    I think it is unintuitive.
  • If std target declaration is after the fastgltf::module target declaration, CMake configure failed, because fastgltf target links std module but does not export the std target.
    CMake Error: install(EXPORT "fastgltf-targets" ...) includes target "fastgltf" which requires target "std" that is not in any export set.
    CMake Error: install(EXPORT "fastgltf-targets" ...) includes target "fastgltf" which requires target "std.compat" that is not in any export set.
    

In my previous approach, I did not declare fastgltf_module CMake target in the project itself and, but in the user project(fastgltf-module-test). Since two targets are separated, I can enable the std module between two targets, and successfully build. I think this approach is the most used among the module support projects like Vulkan-Hpp, glm and other: declaring module target is the responsibility of the user.

Another reason I worried about is: will package managers like vcpkg maintain module target? For same case, the library fmt cannot be used with module in vcpkg, because vcpkg disabled the related CMake options.

Of course there is no general rule for it. As module user population grow up, many CMake based project will expose the module target, so the choice is up to you.

Anyway, could you share me your local module build project? I'm curious how std target only affects to fastgltf_module but not fastgltf target.

@spnda
Copy link
Owner

spnda commented May 14, 2024

Anyway, could you share me your local module build project? I'm curious how std target only affects to fastgltf_module but not fastgltf target.

I just used your fastgltf-module-test repository locally, but didn't use the import std method with your CMake library. I use Clang 18.1 locally with CMake 3.28.1. I don't use import std locally since your library gives this error when configuring:

CMake Error at /Users/sean/Applications/CLion.app/Contents/bin/cmake/mac/aarch64/share/cmake-3.28/Modules/ExternalProject.cmake:1672 (message):
  Do not know how to extract '/modules/c++/v1/' -- known types are: .7z,
  .tar, .tar.bz2, .tar.gz, .tar.xz, .tbz2, .tgz, .txz and .zip
  • How is the new version any different to the old one? I don't really see why it's breaking. You configured the std target before the fastgltf target, too. Though that looks more like a limitation of how your library works, and can be easily fixed by just importing fastgltf before including your library. Those can both be documented in the README of your library. I don't think CMake's own functionality for importing the sodlib module will have this limitation, but correct me if I'm wrong.

  • The errors about install you're getting are just because your std library does not provide any install commands. If someone wants to install fastgltf, CMake needs to know how to install your std library, too. Not sure how to go about that, though, since I think it's statically linked into any executable/library anyway?

@spnda
Copy link
Owner

spnda commented May 14, 2024

Just tested locally with a CMake 3.30 nightly release, and using import std works fine on MSVC. The interface will hopefully change when CMake 3.30 releases, but it should show that the module target order is not relevant to CMake.

cmake_minimum_required(VERSION 3.29.20240513)

# Needs to happen before the project declaration, or C++ language support.
set(CMAKE_EXPERIMENTAL_CXX_IMPORT_STD "0e5b6991-d74f-4b3d-a41c-cf096e0b2508")

project(fastgltf-module-test LANGUAGES CXX)

set(CMAKE_CXX_STANDARD 23)
set(CMAKE_CXX_MODULE_STD 1) # Required for 'import std'

option(FASTGLTF_USE_STD_MODULE "" ON)
add_subdirectory("E:/git/spnda/fastgltf/" fastgltf)

add_executable(fastgltf-module-test main.cpp)
target_link_libraries(fastgltf-module-test PRIVATE fastgltf::module)

@stripe2933
Copy link
Contributor Author

stripe2933 commented May 15, 2024

Just tested locally with a CMake 3.30 nightly release, and using import std works fine on MSVC. The interface will hopefully change when CMake 3.30 releases, but it should show that the module target order is not relevant to CMake.

Ok, my library is just a stopgap implementation for CMake 3.30's feature, so it may result incorrect behavior. I'll investigate it.


It looks like Clang does not accept export for redeclared classes. CI error MSVC works well.

(+) Moving export to symbols' first declaration solves the issue. I'll push the changes.

@spnda spnda merged commit 5c6a9c5 into spnda:main May 15, 2024
@spnda
Copy link
Owner

spnda commented May 15, 2024

@stripe2933 Thank you very much for proposing this and the initial changes, and the last fixes. I've merged this now.

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