-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add C++ modules support #2291
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
base: master
Are you sure you want to change the base?
Add C++ modules support #2291
Conversation
|
Great work. I also thought about this. The approach is very similiar to what was done to https://github.com/nlohmann/json with nlohmann/json#4799. Then, I noticed ... Is this temporary approach with a file with This approach is not using modules natively but rather as an interface to the original way. Does this method work without disadvantages? |
|
This is the best way to my knowledge to support modules on top of a header-only or header/source library, allowing continued support for older versions while providing newer features as an option. I'm not aware of any disadvantages to it besides a being additional translation unit to compile, but if I am wrong please correct me. The only glaring difference in API is that detail symbols are hidden as they are not exported, but in my opinion that's probably better not to expose detail symbols and flood IDE suggestions with implementation details. |
|
What about compiled libraries? Is it possible to have the traditional method and modules installed in parallel? I am thinking of repositories that ship compiled This is relevant here, https://aur.archlinux.org/packages/cpp-httplib-compiled . |
|
Yes I believe it's possible to use shared/static libraries with modules, all of my modular projects compiled to shared libraries that an executable consumes |
|
@mikomikotaishi, thanks for the fine pull request! It's fantastic, but my concern is that someone needs to update @sum01 @jimmy-park @Tachi107 do you have any thought about this pull request? |
|
I could create a Python script, or some other kinds of automated means of updating, which you could run every time it is updated. Until then I would be OK with maintaining this file, as it is a simple process. Such a script would probably comb through the file and add any symbols not part of a detail or internal namespace, or prefixed with an underscore, etc. However, I am curious why it is not feasible to update the file manually. In case it isn't clear how, one can update the file by adding a |
|
I have also seen some repositories use bots to push some commits too. Potentially one such bot could be set up to automatically populate the module with new changes each time there is a mismatch. I don't know anything about how to set this up, but I have seen this before and it could potentially be a solution (but I think the simplest one is just to run a Python script each time any update to the library happens). |
|
Anyway, I think this could be one such way of automatically updating the module. |
|
@mikomikotaishi thanks for the additional explanation. I am ok with the following your suggestion.
We could automatically generate |
|
OK, that makes sense to me. (I don't know anything about how to run GitHub Actions or write scripts for it however, so I'm afraid the most I can do is create a script for this.) |
|
I'm not sure why there were failing workflows as I didn't change anything in the core library |
|
Never mind, it seems the failing CI is happening upstream too. |
|
If the file can be run during the build process (and if the output consists of machine-generated files, it should *only* run during build time), then the destination directory should be configurable (maybe defaulting to the the current working directory). Even better, if the output is a single file, the script should allow the user to specify the output file itself (full path).
This is because downstreams (like Debian, which is what I maintain the meson build scripts for) may have some requirements on where build products should be stored.
|
|
@Tachi107 CMake needs to know what the output directory is ahead of time to compile the module. How do you propose to solve this? |
|
@yhirose I think there is one possible solution to allow both the directory to be user-specified while still supporting CMake module building, which is probably just to have the Python script generate the CMake file too. I don't know if this is too convoluted or awkward of a design though, so please do tell me your thoughts. |
|
Hi Miko,
On Tue Dec 9, 2025 at 10:46 PM CET, Miko wrote:
@Tachi107 CMake needs to know what the output directory is ahead of time to compile the module. How do you propose to solve this?
You should use CMake's add_custom_command() function to invoke the
script and pass it the output file path
https://cmake.org/cmake/help/latest/command/add_custom_command.html
Meson has a similar function, but I can do that myself after this gets
merged.
|
Tachi107
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some feedback now that I have been able to look at the whole code :)
|
If the following simple approach works, I’m fine with it. I don’t mind exposing the module;
// include all included headers seen in <httplib.h>,
// to prevent them from being re-included in the export block
#include <...>
export module httplib;
export {
#include "../httplib.h"
}Also, please don’t forget to remove any files that are no longer needed, revert the changes in README.md, and add any necessary comments only to the CMake files. (I intentionally avoid mentioning any build systems in the README, as they are optional and not officially supported by cpp-httplib itself, but are kindly maintained by excellent contributors. Thanks for your understanding.) |
|
I did make this change, but we have to re-open the PR for it to appear here. I will update the README as requested. |
5c0e60e to
b3963ad
Compare
|
I've removed the changes to the README, but I did notice that the README mentions how to use the script to split the program into |
|
What if we put all detail namespaces inside an
namespace httplib {
// ...
#ifndef HTTPLIB_MODULE_HIDE_DETAILS
namespace detail {
// details here
}
#endif
// ...
}
module;
#define HTTPLIB_MODULE_HIDE_DETAILS
// all httplib includes
#include <...>
export module httplib;
export {
// Because HTTPLIB_MODULE_HIDE_DETAILS is defined, httplib::detail::* symbols aren't exported
#include "../httplib.h"
}@yhirose do let me know what you think, I think this wouldn't any more difficult to maintain moving forward, we would just have to ensure that |
|
Thanks, will review tomorrow |
|
@mikomikotaishi Please don't change httplib.h or README.md at all. Instead, just focus on creating the simpler httplib.cppm, making the smallest possible changes and adding only the minimum necessary usage to the CMake-related files. Thank you. |
|
Then the PR is basically in a finished state. |
I think the build script itself is fine. Meson has |
modules/httplib.cppm
Outdated
| * but it will have to do until we figure out how to use a script to do it. | ||
| */ | ||
| export { | ||
| #include "../httplib.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned earlier, would it make sense to change this to the split .h, with less implementation details exposed, or does it require the full header?
In case the smaller header can be used instead, it might make sense to make this file a template httplib.cppm.in and configure it from CMake by including the header produced my execute_process() (which should really be changed to add_custom_command(OUTPUT), but that's a separate issue) via @VAR@ substitutions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to do this at some point with the smaller header, seeing as the the library has to be compiled anyway if the module has to be compiled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't prefer including such a lengthy list of #include ..., because it's not easily maintainable... Is the following enough in this file?
export module httplib;
export {
#include "../httplib.h"
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The headers above were included first because it activates the header guard preventing it from being pulled in when the actual httplib.h header is included in the export block. If we didn't do that, the module would export stuff like std::* symbols or other headers pulled in.
This can just be copied in when we generate modules with Python instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is too complex and appears difficult to maintain. I'm hesitant to merge this code unless it can be simplified and the code duplication is resolved. Thanks for your understanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weren't you originally content with a Python script to generate the module on demand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s fine. However, I wasn’t satisfied with the update_modules.py script at all. The script should generate the entire httplib.cppm from scratch, rather than updating the existing file using git diff. This would make the script much simpler.
Also, I would prefer not to commit httplib.cppm to this repository. It should be treated the same as httplib.h and httplib.cc, which are generated by split.py. In other words, httplib.cppm is just a build artifact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yhirose It's now possible just doing cmake -B build -S . -G Ninja -DHTTPLIB_COMPILE=ON -DHTTPLIB_BUILD_MODULES=ON. I hope this is sufficiently simple.
969ea3e to
6da7f0c
Compare
b3963ad to
c3a6537
Compare
|
Might I ask a question? Is the currennt approach good for programme packages? Think of https://aur.archlinux.org/packages?O=0&K=cpp-httplib . Can users who have installed a package use modules and not with one package installed? This is very important because if one software depending on cpp-httplib uses modules and the other not one will need to have two different packages whose files would likely collide. Alternatively formulated, can no modules and modules exist in parallel? |
c3a6537 to
55caecb
Compare
I'm not sure if I entirely understand what you mean. Are you asking if it's possible to |
|
@yhirose Hi, could you please review it again and tell me your thoughts on the current solution? I think this is very simple and should be to your liking. |
This pull request adds support for C++20 modules through CMake. It is enabled by the
HTTPLIB_BUILD_MODULESoption (which requiresHTTPLIB_COMPILEto be enabled, though it probably doesn't have to - I only forced this requirement because it seems to make the most sense to force the library to compile if modules are to be compiled).