-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
mark FileStream.Lock and Unlock as unsupported on macOS #47040
mark FileStream.Lock and Unlock as unsupported on macOS #47040
Conversation
@@ -271,6 +273,7 @@ public virtual void Lock(long position, long length) | |||
LockInternal(position, length); | |||
} | |||
|
|||
[UnsupportedOSPlatform("macos")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably the ref needs to be updated, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what about overrides? e.g. IsolatedStorageFileStream.Lock/Unlock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephentoub excellent points (as usual). I've added missing attributes (ref & overrides) PTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry, I've missed the VB files... let me fix the build and get back to you
The CI failures come from #47374, the PR should be ready for review |
@@ -1000,6 +1000,7 @@ Namespace Microsoft.VisualBasic | |||
End Try | |||
End Sub | |||
|
|||
<UnsupportedOSPlatform("macos")> | |||
Public Function InputString(ByVal FileNumber As Integer, ByVal CharCount As Integer) As String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this API's area owner would prefer to reduce the aggressiveness of marking the whole platform as unsupported, and instead only calling oFile.Lock()
and oFile.Unlock()
when the platform is not MacOS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, mono team has a story for adding more attributes for mac/android/ios, they might want to see this changes cc @marek-safar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That story is for SDKs but we are going to audit libraries as well during #47910 work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM given that all the unit tests are failing with unrelated failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
the both call methods that throw on macOS:
runtime/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Lock.OSX.cs
Lines 8 to 16 in 911b1d3