Fix SystemNative_IsATty P/Invoke missing on iOS/tvOS/MacCatalyst#126302
Fix SystemNative_IsATty P/Invoke missing on iOS/tvOS/MacCatalyst#126302
Conversation
… calling isatty Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/96c22fac-0eb4-4756-ae91-390e30c5c572 Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/715fb05b-348a-414e-9529-e30e3eacbbbb Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/area-system-diagnostics-process |
There was a problem hiding this comment.
Pull request overview
Fixes a runtime P/Invoke failure on iOS/tvOS/MacCatalyst by ensuring SystemNative_IsATty is always present in libSystem.Native, while returning a consistent “not a TTY” result on platforms where TTY semantics don’t apply.
Changes:
- Add an Apple mobile/Catalyst preprocessor guard to
SystemNative_IsATty. - On
TARGET_IOS,TARGET_TVOS, andTARGET_MACCATALYST, return0without callingisatty.
|
|
||
| int32_t SystemNative_IsATty(intptr_t fd) | ||
| { | ||
| #if defined(TARGET_MACCATALYST) || defined(TARGET_IOS) || defined(TARGET_TVOS) |
There was a problem hiding this comment.
@rolfbjarne is this the correct fix? or is pal_console.c not compiled for iOS family because there is simply no terminal there?
There was a problem hiding this comment.
@rolfbjarne is this the correct fix? or is
pal_console.cnot compiled for iOS family because there is simply no terminal there?
It pulls in pal_log.m instead (the .c is only included on macOS):
runtime/src/native/libs/System.Native/CMakeLists.txt
Lines 68 to 94 in 7c91160
The ConsolePal.iOS.cs is pretty barebones, I would look why the p/invoke is even included instead of trying to stub out.
There was a problem hiding this comment.
verify-entrypoints.sh was supposed to catch this, but we haven't turned it on for mobile. would be nice to fix that
There was a problem hiding this comment.
I've also opened #126305 to ensure I don't break any mobile platforms again (I already added tests for Android to avoid that, but missed the catalyst vs ios difference)
There was a problem hiding this comment.
Oh the reference is from Process? Out of curiosity I had a look and...
The PlatformDoesNotSupportProcessStartAndKill at the beginning is exactly iDevices. So this should all be unreachable.
System.Diagnostic.Process already multitargets, and it already did multitarget when people added the if check. Weird. This feels like something that should be a partial class.
There was a problem hiding this comment.
The
PlatformDoesNotSupportProcessStartAndKillat the beginning is exactly iDevices. So this should all be unreachable.
But Process is supported on mac catalyst and not supported on iOS and tvOS:
FWIW we could support it on iOS with my most recent changes (I think: #126097 (comment))
There was a problem hiding this comment.
This feels like something that should be a partial class.
Let's say there is a place for improvement and I am trying really hard to make it happen right now ;P
rolfbjarne
left a comment
There was a problem hiding this comment.
I'm not sure I understand how this is going to fix anything, because only changing SystemNative_IsATty doesn't do anything because SystemNative_IsATty isn't built for these platforms in the first place.
| int32_t SystemNative_IsATty(intptr_t fd) | ||
| { | ||
| #if defined(TARGET_MACCATALYST) || defined(TARGET_IOS) || defined(TARGET_TVOS) | ||
| // there is no terminal on these platforms |
There was a problem hiding this comment.
There's a terminal on Mac Catalyst (but none on iOS or tvOS).
You are right, it's not going to fix the problem and that is why I've asked you to review this PR (#126302 (comment)) as I have no clue about mobile targets |
|
Let's get the tests at least failing first in #126306 |
System.Diagnostics.Processhas a P/Invoke toSystemNative_IsATty, but that symbol is not exported bylibSystem.Nativeon iOS, tvOS, and Mac Catalyst, causing a runtime failure on those platforms.Description
Guard
SystemNative_IsATtyinpal_console.cto unconditionally return0onTARGET_IOS,TARGET_TVOS, andTARGET_MACCATALYST— there is no terminal on these platforms.This follows the same
#if defined(TARGET_MACCATALYST) || defined(TARGET_IOS) || defined(TARGET_TVOS)guard pattern used elsewhere in the native libs (e.g.,pal_icushim_static.c,pal_x509.c).fixes #126299