[libcxx-commits] [PATCH] D156585: [libc++][print] Make `<print>` tests require file system support.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

In D156585#4548488 <https://reviews.llvm.org/D156585#4548488>, @var-const wrote:

> In D156585#4544282 <https://reviews.llvm.org/D156585#4544282>, @Mordante wrote:
>
>> I'm not against this patch, but I would like to investigate a bit further. When `FILE` is not available we will run into issues building the dylib. `src/print.cpp` includes `<print>` which has `FILE` in its interface. This works now because we build the dylib with C++20, but I expect it will fail when we switch to C++23. I have another patch for `print(ostream&...)` this patch  uses `FILE` in the dylib, so that is another potential issue before we switch to C++23. Can you test whether changing building the dylib with C++23 works on the platform in question?
>>
>> I wonder whether `no-filesystem` is intended to mean no `cstdio`. If this patch is not urgent I would like to discuss it when Louis is back. If it's urgent we can finish the patch and discuss afterwards.
>
> It's somewhat urgent -- this patch is necessary to fix our internal build.

As discussed on Discord, I'm happy to approve this now and look at a better solution later.

> If I switch to C++23, I get the `FILE`-related errors while compiling `print.cpp` like you described.

Thanks for the confirmation.

LGTM modulo comments.



================
Comment at: libcxx/test/std/input.output/iostream.format/print.fun/lit.local.cfg:1
+if "no-filesystem" in config.available_features:
+    config.unsupported = True
----------------
var-const wrote:
> Mordante wrote:
> > Why do we need this? AFAIK we normally don't do this. Typically we disable all tests manually.
> This approach is used heavily in `test/std/input.output`, mostly to disable tests if localization is not available (there are 10 existing `lit.local.cfg` files in that directory). I did it this way mostly for consistency.
As discussed on Discord I prefer to use the explicit `// UNSUPPORTED: no-filesystem` to keep tests self-contained.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156585