Skip to content

Fix SystemNative_IsATty P/Invoke missing on iOS/tvOS/MacCatalyst #126302

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

Closed
Copilot wants to merge 3 commits into from
+6 −0
Closed

Fix SystemNative_IsATty P/Invoke missing on iOS/tvOS/MacCatalyst #126302

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8971f87
Initial plan
Copilot Mar 30, 2026
06b2044
Fix SystemNative_IsATty for iOS/tvOS/MacCatalyst: return 0 instead of…
Copilot Mar 30, 2026
e35c251
Improve comment: there is no terminal on iOS/tvOS/MacCatalyst
Copilot Mar 30, 2026
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
6 changes: 6 additions & 0 deletions src/native/libs/System.Native/pal_console.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,13 @@ int32_t SystemNative_GetWindowSize(intptr_t fd, WinSize* windowSize)

int32_t SystemNative_IsATty(intptr_t fd)
{
#if defined(TARGET_MACCATALYST) || defined(TARGET_IOS) || defined(TARGET_TVOS)
Copy link
Copy Markdown
Member

@adamsitnik adamsitnik Mar 30, 2026

Choose a reason for hiding this comment

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

@rolfbjarne is this the correct fix? or is pal_console.c not compiled for iOS family because there is simply no terminal there?

Copy link
Copy Markdown
Member

@MichalStrehovsky MichalStrehovsky Mar 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

@rolfbjarne is this the correct fix? or is pal_console.c not 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

if (CLR_CMAKE_TARGET_APPLE)
list (APPEND NATIVE_SOURCES_OBJC_NO_ARC
pal_autoreleasepool.m
pal_environment.m
pal_searchpath.m
pal_datetime.m)
if (CLR_CMAKE_TARGET_MACCATALYST OR CLR_CMAKE_TARGET_IOS OR CLR_CMAKE_TARGET_TVOS)
list (APPEND NATIVE_SOURCES_OBJC_NO_ARC pal_log.m)
if (CLR_CMAKE_TARGET_MACCATALYST)
list (APPEND NATIVE_SOURCES_OBJC_NO_ARC pal_iossupportversion.m)
else()
list (APPEND NATIVE_SOURCES pal_iossupportversion.c)
endif ()
elseif (CLR_CMAKE_TARGET_OSX)
list (APPEND NATIVE_SOURCES
pal_console.c
pal_log.c
pal_iossupportversion.c)
else()
message(FATAL_ERROR "Unsupported Apple platform")
endif ()
set_source_files_properties(${NATIVE_SOURCES_OBJC_NO_ARC} PROPERTIES COMPILE_FLAGS "-fno-objc-arc ${CLR_CMAKE_COMMON_OBJC_FLAGS}")
list (APPEND NATIVE_SOURCES ${NATIVE_SOURCES_OBJC_NO_ARC})
else()

The ConsolePal.iOS.cs is pretty barebones, I would look why the p/invoke is even included instead of trying to stub out.

Copy link
Copy Markdown
Member

@akoeplinger akoeplinger Mar 30, 2026

Choose a reason for hiding this comment

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

verify-entrypoints.sh was supposed to catch this, but we haven't turned it on for mobile. would be nice to fix that

Copy link
Copy Markdown
Member

@adamsitnik adamsitnik Mar 30, 2026

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Member

@MichalStrehovsky MichalStrehovsky Mar 30, 2026

Choose a reason for hiding this comment

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

Oh the reference is from Process? Out of curiosity I had a look and...

runtime/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs

Lines 363 to 392 in 36e6179

if (PlatformDoesNotSupportProcessStartAndKill)
{
throw new PlatformNotSupportedException();
}
EnsureInitialized();
string? filename;
string[] argv;
IDictionary<string, string?> env = startInfo.Environment;
string? cwd = !string.IsNullOrWhiteSpace(startInfo.WorkingDirectory) ? startInfo.WorkingDirectory : null;
bool setCredentials = !string.IsNullOrEmpty(startInfo.UserName);
uint userId = 0;
uint groupId = 0;
uint[]? groups = null;
if (setCredentials)
{
(userId, groupId, groups) = GetUserAndGroupIds(startInfo);
}
// .NET applications don't echo characters unless there is a Console.Read operation.
// Unix applications expect the terminal to be in an echoing state by default.
// To support processes that interact with the terminal (e.g. 'vi'), we need to configure the
// terminal to echo. We keep this configuration as long as there are children possibly using the terminal.
// Handle can be null only for UseShellExecute or platforms that don't support Console.Open* methods like Android.
bool usesTerminal = (stdinHandle is not null && Interop.Sys.IsATty(stdinHandle))
|| (stdoutHandle is not null && Interop.Sys.IsATty(stdoutHandle))
|| (stderrHandle is not null && Interop.Sys.IsATty(stderrHandle));

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.

Copy link
Copy Markdown
Member

@adamsitnik adamsitnik Mar 30, 2026

Choose a reason for hiding this comment

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

The PlatformDoesNotSupportProcessStartAndKill at 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:

runtime/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs

Line 993 in 508f440

=> (OperatingSystem.IsIOS() && !OperatingSystem.IsMacCatalyst()) || OperatingSystem.IsTvOS();

FWIW we could support it on iOS with my most recent changes (I think: #126097 (comment))

Copy link
Copy Markdown
Member

@adamsitnik adamsitnik Mar 30, 2026

Choose a reason for hiding this comment

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

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

// there is no terminal on these platforms
Copy link
Copy Markdown
Member

@rolfbjarne rolfbjarne Mar 30, 2026

Choose a reason for hiding this comment

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

There's a terminal on Mac Catalyst (but none on iOS or tvOS).

(void)fd;
return 0;
#else
return isatty(ToFileDescriptor(fd));
#endif
}

static char* g_keypadXmit = NULL; // string used to enable application mode, from terminfo
Expand Down
Loading