[libcxx-commits] [PATCH] D119036: [libc++] Fix modules and benchmarks CI builds when incomplete features are disabled

Sat Jan 28 04:23:25 PST 2023

v.g.vassilev added inline comments.

================
Comment at: libcxx/include/module.modulemap:10
 module std [system] {
   export std_config
   // FIXME: The standard does not require that each of these submodules
----------------
ldionne wrote:
> v.g.vassilev wrote:
> > I believe the issue that is being fixed in https://reviews.llvm.org/D119468 can be fixed by removing `export std_config` as clang seems to only be able to re-export submodules. Moreover, `std_config` is purely textual and there is nothing to export anyways.
> > 
> > We see such issue on our infrastructure and the suggested fix seem to work.
> > 
> > @ldionne, @aprantl, would it make sense to try it instead, as we should probably really treat `__config` and `__config_site` as textual?
> If it fixes an issue you are seeing, a patch is welcome. It would be nice if there was a way to reproduce the issue you are seeing, though.
https://reviews.llvm.org/D119468 fixes it but my claim is that's the wrong fix as `__config` and `__config_site` are macro-only headers. Do we expect defining a macro in the includer to change the contents of these headers?

If we do they should be treated just like the assert header file, i.e. always marked as `textual` header file. We could protect the headers which must be `textual` similar to what we already do in llvm: https://github.com/llvm/llvm-project/blob/d94a315ee3ecf0fb03cf1c7422e120d9d0e95d00/llvm/cmake/modules/HandleLLVMOptions.cmake#L605-L610

We track the issue here https://github.com/root-project/root/issues/10861 The issue seems quite similar to https://www.mail-archive.com/llvm-bugs@lists.llvm.org/msg51401.html

I added a patch here: https://reviews.llvm.org/D142805

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119036/new/

https://reviews.llvm.org/D119036