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.

Sign up for GitHub

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

Jump to bottom

[libcxx] Add LIBCXX_HAS_TERMINAL_AVAILABLE CMake option to disable print terminal checks #99259

Merged
merged 3 commits into from
Aug 21, 2024
Merged

[libcxx] Add LIBCXX_HAS_TERMINAL_AVAILABLE CMake option to disable print terminal checks #99259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
670192d
[libcxx] Make terminal check always false for `print` on GPU
jhuber6 Jul 16, 2024
edb96ee
Address comments
jhuber6 Aug 5, 2024
95ff1b9
address comments
jhuber6 Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/libcxx-build-and-test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ jobs:
'generic-no-experimental',
'generic-no-filesystem',
'generic-no-localization',
'generic-no-terminal',
'generic-no-random_device',
'generic-no-threads',
'generic-no-tzdb',
Expand Down
3 changes: 3 additions & 0 deletions libcxx/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,8 @@ option(LIBCXX_ENABLE_UNICODE
"Whether to include support for Unicode in the library. Disabling Unicode can
be useful when porting to platforms that don't support UTF-8 encoding (e.g.
embedded)." ON)
option(LIBCXX_HAS_TERMINAL_AVAILABLE
"Build libc++ with support for checking whether a stream is a terminal." ON)
option(LIBCXX_ENABLE_WIDE_CHARACTERS
"Whether to include support for wide characters in the library. Disabling
wide character support can be useful when porting to platforms that don't
Expand Down Expand Up @@ -744,6 +746,7 @@ config_define_if(LIBCXX_ABI_FORCE_ITANIUM _LIBCPP_ABI_FORCE_ITANIUM)
config_define_if(LIBCXX_ABI_FORCE_MICROSOFT _LIBCPP_ABI_FORCE_MICROSOFT)
config_define_if_not(LIBCXX_ENABLE_THREADS _LIBCPP_HAS_NO_THREADS)
config_define_if_not(LIBCXX_ENABLE_MONOTONIC_CLOCK _LIBCPP_HAS_NO_MONOTONIC_CLOCK)
config_define_if_not(LIBCXX_HAS_TERMINAL_AVAILABLE _LIBCPP_HAS_NO_TERMINAL)
if (NOT LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION STREQUAL "default")
config_define("${LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION}" _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION)
endif()
Expand Down
5 changes: 5 additions & 0 deletions libcxx/cmake/caches/Generic-no-terminal.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
set(LIBCXX_HAS_TERMINAL_AVAILABLE OFF CACHE BOOL "")

# Speed up the CI
set(LIBCXX_TEST_PARAMS "enable_modules=clang" CACHE STRING "")
set(LIBCXXABI_TEST_PARAMS "${LIBCXX_TEST_PARAMS}" CACHE STRING "")
1 change: 1 addition & 0 deletions libcxx/include/__config_site.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#cmakedefine _LIBCPP_ABI_FORCE_MICROSOFT
#cmakedefine _LIBCPP_HAS_NO_THREADS
#cmakedefine _LIBCPP_HAS_NO_MONOTONIC_CLOCK
#cmakedefine _LIBCPP_HAS_NO_TERMINAL
#cmakedefine _LIBCPP_HAS_MUSL_LIBC
#cmakedefine _LIBCPP_HAS_THREAD_API_PTHREAD
#cmakedefine _LIBCPP_HAS_THREAD_API_EXTERNAL
Expand Down
2 changes: 1 addition & 1 deletion libcxx/include/print
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ _LIBCPP_HIDE_FROM_ABI inline bool __is_terminal([[maybe_unused]] FILE* __stream)
// the behavior in the test. This is not part of the public API.
# ifdef _LIBCPP_TESTING_PRINT_IS_TERMINAL
return _LIBCPP_TESTING_PRINT_IS_TERMINAL(__stream);
# elif _LIBCPP_AVAILABILITY_HAS_PRINT == 0
# elif _LIBCPP_AVAILABILITY_HAS_PRINT == 0 || defined(_LIBCPP_HAS_NO_TERMINAL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you run the test suite with LIBCXX_HAS_NO_TERMINAL, I assume you see at least a few tests failing, right? At least some tests for std::print should be failing. We would need to add something similar to

llvm-project/libcxx/utils/libcxx/test/features.py

Line 383 in d1b2940

"_LIBCPP_HAS_NO_UNICODE": "libcpp-has-no-unicode",
so that the test suite can recognize the lack of a terminal and XFAIL or UNSUPPORTED the right tests based on it.

For ultimate consistency, I think I would request that you actually add a new CI job for this setting. You can look at what we're doing in e.g.

llvm-project/libcxx/utils/ci/run-buildbot

Line 467 in d1b2940

generic-no-localization)
and then

llvm-project/.github/workflows/libcxx-build-and-test.yaml

Line 148 in d1b2940

'generic-no-localization',

In practice, it's probably way overkill to add an actual CI configuration just for this tiny setting, but this is what we've been doing and I'd like us to be consistent. I will make it a priority to figure out a story for merging some of these CI jobs together -- I think we probably want to have a nightly build with all the individual configurations, but have the per-PR CI only run a few merged configurations. But anyway, I'll handle that part, you only need to add a new job like we've done for the other carve-outs like this.

Copy link
Contributor Author

@jhuber6 jhuber6 Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to find which tests fail and add a config for it.

How much latency will this add to the bot runs? I feel like what is essentially just ifdef (false) isn't worth the extra five minutes it takes to do a fresh build + test, but I can add it if you want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mordante We should have a test that exercises how we print to a stream (based on whether it's a terminal), and that test should fail (or at least have to be conditionalized) when terminal support is disabled. Do you agree that we're missing coverage right now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some sh tests in /data/src/llvm/libcxx/test/std/input.output/iostream.format/print.fun/. These sh tests are marked // UNSUPPORTED: no-filesystem

@jhuber6 Are you running the test with filesystem disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was done with a default x64 Linux build, so it should've had filesystem unless that's off by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'm assuming since this patch adds it to the CI it'd at least be caught pretty fast if I'm incorrect.

return false;
# elif defined(_LIBCPP_WIN32API)
return std::__is_windows_terminal(__stream);
Expand Down
5 changes: 5 additions & 0 deletions libcxx/utils/ci/run-buildbot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,11 @@ generic-no-localization)
generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-no-localization.cmake"
check-runtimes
;;
generic-no-terminal)
clean
generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-no-terminal.cmake"
check-runtimes
;;
generic-no-unicode)
clean
generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-no-unicode.cmake"
Expand Down
1 change: 1 addition & 0 deletions libcxx/utils/libcxx/test/features.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,7 @@ def _mingwSupportsModules(cfg):
"_LIBCPP_HAS_NO_FILESYSTEM": "no-filesystem",
"_LIBCPP_HAS_NO_RANDOM_DEVICE": "no-random-device",
"_LIBCPP_HAS_NO_LOCALIZATION": "no-localization",
"_LIBCPP_HAS_NO_TERMINAL": "no-terminal",
"_LIBCPP_HAS_NO_WIDE_CHARACTERS": "no-wide-characters",
"_LIBCPP_HAS_NO_TIME_ZONE_DATABASE": "no-tzdb",
"_LIBCPP_HAS_NO_UNICODE": "libcpp-has-no-unicode",
Expand Down
Loading