Add modules support #1015
Merged
Add modules support #1015
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…o 6.0.0, fixing this
Directly go to `modules/ftxui`
No longer needed
|
Thanks! I am enthousiastic. Things to keep in mind. FTXUI supports C++11, so it must not break those users. A priori, it is possible to support both. To provide a review, I would need to spend some time learning about C++ module, to ensure this patch is the best we can do, without regrets. It will take me some time. |
I think it would be best if modules were just opt-in, so that those using C++20 can benefit from them while the builds using pre-C++20 do not break either. I can make this change, and the library should be compatible for all versions. |
|
I got an error: This is problematic, because it means all FTXUI users will have this error. We should probably try to detect what cmake generator is in use and enable modules when supported. Then I tried: cmake .. -DCMAKE_GENERATOR:INTERNAL=Ninjabut it doesn't work: I see I need to install Maybe this is too early to support C++ modules by default? Alternatively, maybe we should put this behind a build flag. |
|
I put it behind a flag that should now only build if the flag |
|
Hi, are there any changes you're still looking for before you can accept the pull request and have it merged? |
|
I'm actually now wondering if |
|
Thanks! I am going to:
I don't have a lot of time, so please expect ~1 month before I can fully merge this patch. I don't want to rush support this. |
|
I made an update:
Tasks (Feel free to grab)
|
|
I think C++ modules require CMake 3.28, is there a reason you lowered the version requirement? |
Increasing the cmake version would prevent a lot of users from using FTXUI. It can be conditionnal on |
|
I am supportive of the idea of having a single By the way, the two additional repositories adding additional components, I don't know if you plan on eventually integrating these into the main repo, but for now I do have a template on my own project for a |
|
I thought I just fixed that // Elements.cppm
export namespace ftxui {
// ...
using ftxui::size;
using ftxui::frame;
using ftxui::xframe;
using ftxui::yframe;
// ...
}https://github.com/mikomikotaishi/FTXUI/blob/main/src/ftxui/dom/Elements.cppm |
oh sorry, i didn't saw new commits |
|
Hi @ArthurSonzogni, is there still any interest in merging this commit? The CI tests pass and everything should be correctly exported. Please let me know if there are any concerns. |
|
I'm not sure why, but whenever I merge incoming changes, the documentation tests fail, but all the other build-related tests pass. |
|
Is there anything we still have left to do before we can merge? |
I think we are 2 errors away from being able to build all the examples: Error 1 Error 2 |
|
I pushed one change to the documentation to clarify some information regarding module organisation. |
|
Is that all the issues fixed? I think we should be ready to merge now, then? |
|
Yes, everything is good. I am going to make minor tweaks and merge the patch. Thanks for your work on this! |
|
Thank you very much!!! I have wanted this for a long time! |
|
I'm glad to hear you like it! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have tried making a C++ modules version for FTXUI, using modules
ftxui.*, which is built if CMake version is above 3.28. I've tested these modules on my own projects and they work fine, and I'm sure most C++20+ users would like the ability to import FTXUI as a module.FTXUI is now importable with: