Skip to content

Add proper C++20 module support - #4799

Merged
nlohmann merged 23 commits into
nlohmann:developfrom
mikomikotaishi:develop
Jun 29, 2025
Merged

nlohmann merged 23 commits into
nlohmann:developfrom
mikomikotaishi:develop

Conversation

@mikomikotaishi

@mikomikotaishi mikomikotaishi commented May 29, 2025 •

Copy link
Copy Markdown
Contributor
  • The changes are described in detail, both the what and why.
  • If applicable, an existing issue is referenced.
  • The Code coverage remained at 100%. A test case for every new line of code.
  • If applicable, the documentation is updated.
  • The source code is amalgamated by running make amalgamate.

This pull request adds proper support through CMake for a module for the nlohmann-json library, which is exported by the module nlohmann.json. It also updates the existing test (created from this pull request) to use that module (the original name in that was json, however nlohmann.json is probably a more appropriate name as it is less likely to cause naming conflicts). The module must be built using NLOHMANN_JSON_BUILD_MODULES in CMake, and this is not enabled by default.

I have also gone ahead to update the website and documentation to make note of this change.

@coveralls

coveralls commented May 29, 2025 •

Copy link
Copy Markdown

Coverage Status

coverage: 99.19%. remained the same
when pulling b2d76be on mikomikotaishi:develop
into d70e46b on nlohmann:develop.

@mikomikotaishi

mikomikotaishi commented May 29, 2025 •

Copy link
Copy Markdown
Contributor Author

The CI test for modules does not pass but I am afraid of making changes to the CI configuration that could break something. However, compiling the library manually seems to be fine, by just using cmake -S . -B build -G Ninja and cmake --build build and having the macro enabled.

Comment thread README.md Outdated
Comment thread README.md Outdated
@mikomikotaishi

Copy link
Copy Markdown
Contributor Author

@nlohmann Hi, so I believe I have addressed all of your concerns (at least the ones you have posted up to this point). Is there anything else you need me to address, and if not, can these changes now be merged?

@nlohmann

Copy link
Copy Markdown
Owner

Please check the CI issues.

Signed-off-by: Toyosatomimi no Miko <110693261+mikomikotaishi@users.noreply.github.com>
Signed-off-by: Toyosatomimi no Miko <110693261+mikomikotaishi@users.noreply.github.com>
Signed-off-by: Toyosatomimi no Miko <110693261+mikomikotaishi@users.noreply.github.com>
Signed-off-by: Toyosatomimi no Miko <110693261+mikomikotaishi@users.noreply.github.com>
Signed-off-by: Toyosatomimi no Miko <110693261+mikomikotaishi@users.noreply.github.com>
Signed-off-by: Miko <110693261+mikomikotaishi@users.noreply.github.com>
Signed-off-by: Toyosatomimi no Miko <110693261+mikomikotaishi@users.noreply.github.com>
Signed-off-by: Toyosatomimi no Miko <110693261+mikomikotaishi@users.noreply.github.com>
Signed-off-by: Toyosatomimi no Miko <110693261+mikomikotaishi@users.noreply.github.com>
Signed-off-by: Toyosatomimi no Miko <110693261+mikomikotaishi@users.noreply.github.com>
Signed-off-by: Toyosatomimi no Miko <110693261+mikomikotaishi@users.noreply.github.com>
@github-actions github-actions Bot added the CI label May 29, 2025
@mikomikotaishi

Copy link
Copy Markdown
Contributor Author

Please check the CI issues.

I have re-signed my commits, but the check workflow requires approval.

Signed-off-by: Toyosatomimi no Miko <110693261+mikomikotaishi@users.noreply.github.com>
@mikomikotaishi

mikomikotaishi commented May 29, 2025 •

Copy link
Copy Markdown
Contributor Author

@nlohmann Hi, so it seems that some of the tests (specifically ci_static_analysis_clang (ci_test_clang) and ci_cmake_options (ci_test_legacycomparison)) are failing due to files that I have not modified, and I do not know how to resolve these CI issues, as I have very little experience working with GitHub's CI system. If you have any idea what is happening, could you help with resolving those issues? I have tried to resolve specifically the CI test that corresponds to the changes I made (ci_module_cpp20 (silkeh/clang:latest)), however it takes almost 50 minutes for the GitHub CI to begin those tests.

Regardless, I can confirm that the C++ modules function correctly when compiling myself. The steps I have taken are:

  • Run cmake -S . -B build -G Ninja -DNLOHMANN_JSON_BUILD_MODULES=ON
  • Run cmake --build build
    And I confirm that this works on CMake 4.0.2 on Clang 19.1.7. (I am aware that there are apparently issues compiling this library on MSVC, however as I am on Linux I have no ability to test MSVC.)

@nlohmann

Copy link
Copy Markdown
Owner

I will check tomorrow.

@nlohmann

Copy link
Copy Markdown
Owner

I have a fix for ci_static_analysis_clang in #4801. Once this is merged, please rebase and check Ubuntu / ci_module_cpp20 (gcc:latest) (pull_request).

Signed-off-by: Toyosatomimi no Miko <110693261+mikomikotaishi@users.noreply.github.com>
@mikomikotaishi

Copy link
Copy Markdown
Contributor Author

Hmm, I had a look and I think I might understand what the issue was with the modules test, it appears I had to modify the test in ubuntu.yml rather than in codeql-analysis.yml. Let's see what happens

Signed-off-by: Toyosatomimi no Miko <110693261+mikomikotaishi@users.noreply.github.com>
@mikomikotaishi

Copy link
Copy Markdown
Contributor Author

#4801 is merged. Please rebase.

Rebased and addressed issues except for modules/ being in the project root, I am not sure if you have a better location in mind

Comment thread CMakeLists.txt Outdated
Comment thread cmake/ci.cmake Outdated
Comment thread docs/mkdocs/docs/features/modules.md Outdated
Comment thread tests/module_cpp20/CMakeLists.txt Outdated
Comment thread tests/module_cpp20/CMakeLists.txt Outdated
Comment thread modules/CMakeLists.txt Outdated
mikomikotaishi and others added 2 commits June 1, 2025 18:13
Co-authored-by: Niels Lohmann <niels.lohmann@gmail.com>
Signed-off-by: Miko <110693261+mikomikotaishi@users.noreply.github.com>
Signed-off-by: Toyosatomimi no Miko <110693261+mikomikotaishi@users.noreply.github.com>
@mikomikotaishi

Copy link
Copy Markdown
Contributor Author

Changes made as requested

@nlohmann nlohmann left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Some comments which may stem from my little experience with modules.

Comment thread src/modules/CMakeLists.txt Outdated
Comment thread src/modules/CMakeLists.txt Outdated
Comment thread tests/module_cpp20/CMakeLists.txt Outdated
Comment thread tests/module_cpp20/CMakeLists.txt Outdated
Comment thread tests/module_cpp20/CMakeLists.txt Outdated
Signed-off-by: Toyosatomimi no Miko <110693261+mikomikotaishi@users.noreply.github.com>
Signed-off-by: Toyosatomimi no Miko <110693261+mikomikotaishi@users.noreply.github.com>
@mikomikotaishi

mikomikotaishi commented Jun 2, 2025 •

Copy link
Copy Markdown
Contributor Author

That should be all concerns addressed, also, I was thinking that potentially in the future if anything from namespace nlohmann::detail:: needed to be used by the client, an additional module nlohmann.json.detail could be created, which may or may not be exported by the main module nlohmann.json

@mikomikotaishi

Copy link
Copy Markdown
Contributor Author

Is it ready to be merged?

@nlohmann

nlohmann commented Jun 3, 2025

Copy link
Copy Markdown
Owner

I need to review the CMake part once more. Please be patient.

@mikomikotaishi

Copy link
Copy Markdown
Contributor Author

What did you think of the idea for the future of exporting implementation contents (nlohmann::detail::) in nlohmann.json.detail?

@nlohmann

nlohmann commented Jun 3, 2025

Copy link
Copy Markdown
Owner

The detail namespace is not meant to be used by client code. It is allowed to be changed or removed between releases without warning.

@mikomikotaishi

Copy link
Copy Markdown
Contributor Author

@nlohmann Hi, is this pull request still planned to be merged?

@nlohmann

Copy link
Copy Markdown
Owner

I am traveling right now and will check this next week

@mikomikotaishi

Copy link
Copy Markdown
Contributor Author

Understood, thanks for letting me know

Comment thread tests/module_cpp20/CMakeLists.txt

@nlohmann nlohmann left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looks good to me.

@nlohmann nlohmann added this to the Release 3.12.1 milestone Jun 29, 2025
@nlohmann
nlohmann merged commit b7f7411 into nlohmann:develop Jun 29, 2025
@nlohmann

Copy link
Copy Markdown
Owner

Thanks for the PR and the patience!

@mikomikotaishi

Copy link
Copy Markdown
Contributor Author

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants