Conversation
|
@vinniefalco Seeing as you are the last Boost organisation member to interact with the library that I can see, may I request a review from you? |
|
I'm not sure why the Windows builds are failing, seeing as I haven't touched anything to do with the code itself. I don't expect it to fail, especially when the others do pass - I mostly notice the problem is with a missing file, that I didn't touch? |
|
I am not really qualified to review this but I will find someone who is ! Thank you |
251fc9c to
5982688
Compare
|
@vinniefalco Hi, any update on anyone who could review this PR? |
|
@ashtum ? |
|
Sorry for the late reply, and thank you @mikomikotaishi for your PR. There is ongoing research into making Boost consumable as C++20 modules, led by Rubén Pérez. The latest updates are available in his blog post: https://anarthal.github.io/cppblog/modules3 Your work does not appear to be based on that effort, and if I’m not mistaken, it technically couldn’t be, since it requires modularizing libraries in dependency order. Given that, are you proposing this PR as a temporary solution? If so, as a Beast user, what specific problem does this solve for you? Is this something that could instead be done at the application level as a workaround? |
|
Hi, While I haven't been familiar with that blog post prior nor did I explicitly copy its formula to build this PR, I do think that the contents of my PR resemble the following module interface that was described: // File: boost_mp11.cppm
module; // Global module fragment
#define BOOST_MP11_INTERFACE_UNIT // We want headers to actually declare entities
// No BOOST_CONFIG_SKIP_IMPORT_STD: import std is fine in the global module fragment
#include <cassert> // Some standard library headers need to be included for their macros
#include <boost/mp11.hpp> // All entities are attached to the global module.
export module boost.mp11;
// List all symbols we want to export
export namespace boost::mp11 {
using mp11::list;
}This technique has pros and cons, and these can be ironed out as more users adopt or give feedback on the feature. Nonetheless, when you say "as a temporary solution", I think that this does provide some structure or means to use the library as a module, and this can be developed or made to conform more with the blog post's design goals, so I'd be happy to see this provided for the time being. A lot of my projects have been using modules as opposed to headers, and I find that once build system configuring and tooling is correctly configured it is nicer to use than traditional headers. While many things could be done at the application level (I've done this a lot, for example with asio/boost::asio, nlohmann::json, and other libraries, etc.), this feature does seem to have interest from others, such as building a better environment for module adoption where popular libraries can be easily supported with modules, and thus by just providing it with first-class support we would be able to make a lot of lives easier. |
Is this part addressed in your PR:
I believe the modularization was postponed due to a lack of proper tooling, but it may pick up again in the next release cycle. For now, a workaround on your side while waiting for the final solution might be the best option. @anarthal, any thoughts? |
I don't think what this is prescribing against applies here, because the concern seems to be with accidentally exporting non-Boost symbols from the module. This isn't a problem because we manually exported each symbol. Of course, we can change this later in the future.
Would it be unreasonable to ask that this could be merged into a separate branch for the time being? |
|
Yup, jumping in into this discussion now. I wrote the linked blog post some time ago. Some important things I see here: Running tests My advice would be to run a decent amount of the test suite using the module before merging it. At least, I'd target the tests that exercise the public API. I don't think it's required by now to run those testing implementation details. The reason why I suggest this is because Running tests requires you to:
import std My personal view is that TL;DR: I think there are 3 options here:
IMO 2 sounds the most reasonable to me, with 1 being doable if the maintainers like risk. Let me know if you go with 2 and need to get things reviewed/need assistance with CI. |
|
I wouldn't mind working towards number 2, but I'm not very skilled with CI manipulation and have worked with it very seldom. import std is very difficult to actually get working, so much so that I have my own "fake" std module just because I have never managed to get the official one to work on CMake. |
|
I feel you with |
|
In my experience Clang has much better support than GCC, so I'll probably try and use Clang for CI |
|
@vinniefalco Is option 2 reasonable with you, that we work on setting up CI tests for this PR and then merge? |
|
its up to Mohammad |
|
cc. @ashtum |
How are you currently consuming Beast as a dependency? Are you cloning the superproject with submodules, or downloading a Boost CMake release from https://github.com/boostorg/boost/releases? As far as I know, the official releases and installed versions don’t yet include proper CMake support for consuming the libraries as C++20 modules. Since any workaround would be temporary, I’d recommend applying these changes in your own branch for now, until the libraries are modularized in dependency order. |
|
I am confused what you mean by "modularized in dependency order". Is there a specific requirement that the Boost libraries this library depends on must be moduarlised before |
Yes, if we want to make |
This PR adds support for C++20 modules through CMake, as
boost.beast. The exported classes are those listed in the reference documentation.This is hopefully in line with other Boost libraries which have begun offering modules:
boost.anyboost.pfrboost.regexboost.type_index