spnda / fastgltf

A modern C++17 glTF 2.0 library focused on speed, correctness, and usability
https://fastgltf.readthedocs.io/v0.8.x/
MIT License
306 stars 45 forks source link

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

Closed stripe2933 closed 6 months ago

stripe2933 commented 6 months ago

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

stripe2933 commented 6 months ago

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.

spnda commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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:

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 commented 6 months ago

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
spnda commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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