Skip to content

Conversation

@BCSharp
Copy link
Member

@BCSharp BCSharp commented Jan 10, 2025

This adds platform guards to relevant functions in PythonNT. By keeping them in a separate file I hope it will be easier to prevent omissions and inadvertent loads of Mono.Unix on Windows. Also, it makes nt.cs just a little bit smaller, which is always a good thing.

@BCSharp BCSharp marked this pull request as draft January 10, 2025 07:34
}

if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) {
if (!isWindows) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really an issue since we're not calling native APIs here, but any idea if the platform analyzer would pick up on an isWindows variable and guard against Windows calls?

Copy link
Member Author

Choose a reason for hiding this comment

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

As it is written now, no platform-specific calls are allowed in CheckFileAccessAndSize, regardless whether they are conditioned by isWindows in any way. .NET has SupportedOSPlatformGuardAttribute, but it cannot be applied to a parameter.

The reason why I replaced the platform check with a parameter is because having the platform test in this function was invalidating the platform context after the first call site, and I needed to make a call to PythonNT.dupUnix.

Copy link
Contributor

@slozier slozier Jan 11, 2025

Choose a reason for hiding this comment

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

Cool, didn't know (or forgot) that SupportedOSPlatformGuardAttribute existed.

Weird that iy breaks the platform check. Alternatively the parameter could be called allowGrow or something.

@BCSharp BCSharp marked this pull request as ready for review January 11, 2025 05:35
Copy link
Contributor

@slozier slozier left a comment

Choose a reason for hiding this comment

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

LGTM

public static class PythonWinApi {
#region Public API

[SupportedOSPlatform("windows")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the attribute on both the class and method?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's redundant.

@BCSharp BCSharp merged commit 1cd8e71 into IronLanguages:main Jan 13, 2025
8 checks passed
@BCSharp BCSharp deleted the os_posix branch January 15, 2025 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants