Skip to content
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

Proper fmt and std modules support (starting with v11.0.0) #5

Open
3 of 5 tasks
Klaim opened this issue Jul 1, 2024 · 12 comments
Open
3 of 5 tasks

Proper fmt and std modules support (starting with v11.0.0) #5

Klaim opened this issue Jul 1, 2024 · 12 comments

Comments

@Klaim
Copy link
Collaborator

Klaim commented Jul 1, 2024

Version v11.0.0 of fmt is now correctly supported using CMake and also provides a way to make fmt use std if import std; is working. We need to

  • fix current fmt modules support (it's broken and limited in several ways)
    • there is a missing mechanism so that the tests can know if they need to set cxx.features.modules = true when config.fmt.enable_modules as been set to true for the fmt package and not a configuration-wide value. I started experimenting with a solution this several weeks ago but had to stop and couldnt resume exploring this yet.
    • currently the lib{fmt} target will expose either headers OR a module but not both. The recent upstream version however does support providing both in the same target because there is an optional support for having the fmt module's code using external "C++" { which makes it part of the global module instead of the fmt module. This allows code that includes and imports fmt to still work (with consequences on the link-side). Ideally, we would support both exclusively-module, exclusively headers and "both" cases. That should be easy to provide. We also need tests for that "both" case.
  • add a configuration option to enable using import std; inside the fmt module (new v11 feature, see upstream release note)
  • add configurations in the manifests to run the tests using the modules versions of fmt on build2's CI
@Klaim Klaim mentioned this issue Jul 1, 2024
3 tasks
@kamrann
Copy link
Contributor

kamrann commented Jul 15, 2024

Happy to help if needed. What are the current issues with the modules support?

@Klaim
Copy link
Collaborator Author

Klaim commented Jul 15, 2024

@kamrann Thanks! I just clarified the missing/incomplete parts in the first post to have a clear todolist. Feel free to ask for details and clarifications.
Unfortunately I lack time to do all this in one go so any help is good.

Note that upstream tests are not there yet and the tests I'm talking about above are the "smoke tests" currently existing, which are already setup to be able to run the same code being included or imported, but we need to improve that.

@kamrann
Copy link
Contributor

kamrann commented Jul 15, 2024

Ok thanks, no worries. Yeah I think I should be able to help with some of this. Though the question of interaction between and usage of cxx.features.modules and a package-specific config variable for modules is something I've been wondering about myself lately and am still not entirely sure on.

So to try to clarify my understanding of the intentions with your existing setup. I also in my experiments intuitively added a config.my_lib.enable_modules variable, but recently have been questioning the reasons for having this on top of the built-in cxx.features.modules. I guess fundamentally the idea is to distinguish "building with modules enabled" from "building this library for consumption as module(s)"? It feels like making this distinction is largely only valuable due to the current state of modules support, where we might want to disable or tweak the modules build to workaround some compiler bug? Other than that it's not clear to me what reason there would be for wanting to build a package with modules enabled but explicitly disable building a module for the package itself. Though perhaps there are nuances when it comes to installation, which is something I've not yet thought much about.

So I suppose what I'm getting at is, there are rather a lot of potential combinations when it comes to modularizing (cxx.features.modules, building fmt as a module, enabling import std, the external "C++" toggle) and it would be good to have a clear idea of which combinations of these things actually make sense before trying to support every possible combination.

@Klaim
Copy link
Collaborator Author

Klaim commented Jul 15, 2024

I have been thinking about all this too. Note that I disagree with the current way the option is initialized too (so I disagree with my old self). (https://github.com/build2-packaging/fmt/blob/master/fmt/build/root.build#L18) This init needs to be removed.

So I suppose what I'm getting at is, there are rather a lot of potential combinations when it comes to modularizing (cxx.features.modules, building fmt as a module, enabling import std, the external "C++" toggle) and it would be good to have a clear idea of which combinations of these things actually make sense before trying to support every possible combination.

My thinking at the moment:

  • When a build configuration has cxx.features.modules == true, it doesnt mean that the code will import fmt;, it could still prefer the includes, or depend on code that does. However, we could simply allow both by default.
  • The module ownership choice does make a difference in how language entities are bound to modules in the language and that could have an impact in the build/link/symbols aspects, but at the moment I'm not sure how important that difference is.
  • The choice of if fmt will or not use import std; has an impact on build perfs of the overall project so to me it's important and orthogonal to all other concerns listed here. Unfortunately we cannot decide automatically that as there is no easy way to detect if it's possible to do import std; given a build configuration (AFAIK). If we had a way to detect that, we could just allow import std; as long as it's possible to use in the given build configuration, as there is no benefit for the alternative in that case.
  • I dont think there are other combinations existing or interesting, module-wise (I'm ignoring whatever options from fmt's CMake that is not supported here yet).

Given that, I propose that:

  1. We remove config.fmt.enable_modules, it loses it's meaning (even if it matches upstream).
  2. When cxx.features.modules == true we automatically provide by default the fmt module with the "both include and import" feature enabled -> the user's code will decide to import or include or both, and no need for an extra step to use fmt;
  3. (maybe) we provide an option to force only the module (no possible include from the lib - impl is as it is currently done)
  4. we provide an option for the import std; usage, false by default (except if we find a way to determine if it's possible to use);

Point 3 could be set aside for later as it is not proven important yet. Just note that today if I had that option, I would systematically turn it on, personally.

@kamrann
Copy link
Contributor

kamrann commented Jul 16, 2024

That sounds good, though need to clarify one thing on the 'both' mode. Are you saying that under cxx.features.modules == true, we build the fmt module and also automatically provide the preprocessor define for the extern C++ mode? So essentially enforce the most lenient mode in order to minimize configuration complexity? And then 3. would be a way to turn off that define so the ownership reverts to the fmt module. I think it's reasonable, just making sure I'm on the same page in my understanding.

Regarding import std, it appears there is a feature test macro for it. From https://en.cppreference.com/w/cpp/feature_test:

__cpp_lib_modules | Standard library modules std and std.compat

I'm wondering though if it may be worth bringing this up with Boris to see if he intends to integrate something into the toolchain for import std. Since it seems to me that it's something that there would be no real point in configuring on a per-package basis. If the compiler toolchain and stdlib used in the configuration supports it, then it would make sense for it to be enabled configuration-wide, and if not, then obviously disabled configuration-wide. So it would seem useful to have a built-in variable available similar to features.modules, for access from any package buildfile.

One last thing, I assume I should be branching off master? The modules branch looks outdated.

@Klaim
Copy link
Collaborator Author

Klaim commented Jul 16, 2024

I think it's reasonable, just making sure I'm on the same page in my understanding.

You understand correctly the proposition 👍🏽 Thinking about it the only thing that "prevents" is that there would be no way to not provide the modules in that situation (no way to "force" includs only).

We might want to take the time to ponder that proposition for a minute.

Regarding import std, it appears there is a feature test macro for it.

I suspect this is not reliable enough yet but we might experiment to be sure. Also, I'm thinking about build2's support too, which for example doesnt work yet with clang+libc++ on windows, while the feature is available in libc++ when using clang (ignoring build2).

Since it seems to me that it's something that there would be no real point in configuring on a per-package basis.

The idea of that choice is there only because upstream provide that choice, and having it enabled if possible does provide an advantage, I wouldnt consider it otherwise. Hopefully not all libraries will do that and they will probably "just" rely on feature macros instead (and maybe fail with build2 though).

If the compiler toolchain and stdlib used in the configuration supports it, then it would make sense for it to be enabled configuration-wide, and if not, then obviously disabled configuration-wide.

Yes that would be the ideal, but only if the build-system enables import std; if the toolchain provides it, which is not always the case here.

So it would seem useful to have a built-in variable available similar to features.modules, for access from any package buildfile.

Do you mean to detect if import std; is handled by build2 + the toolchain? If so then agree it would be useful. However it's one additional thing to handle on build2's side that might not be worth maintaining long-term, and it's easy to provide an equivalent in libraries that already have the scafholding to do so. So we might want to not wait for such feature too and not add maintenance to build2.
Not sure what's best.

One last thing, I assume I should be branching off master? The modules branch looks outdated.

Yes ignore other brancehs than master, the modules branch was when I implemented the first version, I didnt cleanup the old branches. Or re-use it by moving it to master if you want. I note to do a branch cleanup pass when I find a minute.

@kamrann
Copy link
Contributor

kamrann commented Jul 17, 2024

Do you mean to detect if import std; is handled by build2 + the toolchain?

Yeah, although there are two essentially orthogonal things.

  1. Having a built-in way for a package buildfile to query if import std is available, so it can configure things accordingly (i.e. for packaging something like fmt, adding the preprocessor define used by upstream).
  2. The auto-detection of import std availability.

It would be nice if build2 could provide both, but even forgetting 2, it would be great to have 1 just for consistency in handling this when packaging. It would mean no matter how many dependencies a project had with potential for using import std, it would just need something like bdep sync config.cxx.features.import_std=true and you're done. Without 1, unless I'm missing something you're going to have to manually configure something in every such dependency, each of which may deal with it in slightly different ways too.

about build2's support too, which for example doesnt work yet with clang+libc++ on windows, while the feature is available in libc++ when using clang (ignoring build2)

Curious, do you have specific reason to believe import std should work (ignoring build2) with clang + libc++ on Windows? Reason I ask is that I actually got it working recently, but it involved not only tweaking build2 but also hacking at the Windows system headers. So I assumed the libc++ support was officially constrained to Linux for the moment.

@Klaim
Copy link
Collaborator Author

Klaim commented Jul 17, 2024

Curious, do you have specific reason to believe import std should work (ignoring build2) with clang + libc++ on Windows? Reason I ask is that I actually got it working recently, but it involved not only tweaking build2 but also hacking at the Windows system headers. So I assumed the libc++ support was officially constrained to Linux for the moment.

My assumption is based on libc++'s usual portability on windows but I might be wrong because of the details you found, I wasnt aware you had to change system headers (and that sounds fishy XD).
But my point is that libc++ in c++>=23 does advertise that it supports import std; through __cpp_lib_modules, whatever the platform, and because it's independant from the buildsystem's support, we can't rely on just checking that (unfortunately).

@kamrann
Copy link
Contributor

kamrann commented Jul 22, 2024

Pushed an experimental attempt at your (3) to the PR. My attempt to test it's working as intended feels a bit weird, maybe there is something simpler? Can of course revert that commit if you think it's best to hold off on the functionality for the moment.

@Klaim
Copy link
Collaborator Author

Klaim commented Jul 22, 2024

Thanks, I'll test asap (in the week) 👍🏽

@Klaim
Copy link
Collaborator Author

Klaim commented Aug 7, 2024

For info:

I'm handling some IRL issues that prevents me from focusing on this packaging update plan and general maintenance of packages, so I proposed to @kamrann to take the lead on the maintenance of fmt package from now on. Thanks @kamrann !

@kamrann
Copy link
Contributor

kamrann commented Sep 10, 2024

Status update. We're most of the way there, and at this point mostly waiting on some more upstream changes needed to resolve linker errors with modules, and compiler module support to be generally less broken!

Some initial modules CI configurations have been added to the manifest but remain disabled for now.

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

No branches or pull requests

2 participants