-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[release/6.0] Exclude the managed code around libproc on iOS/tvOS (#61590) #61659
[release/6.0] Exclude the managed code around libproc on iOS/tvOS (#61590) #61659
Conversation
Since libproc is a private Apple API, it is not available on iOS/tvOS and should be excluded (see dotnet#61265 (comment) and above for more details). This PR excludes $(CommonPath)Interop\OSX\Interop.libproc.cs on the iOS/tvOS as well as makes some methods in Process, ProcessManager, and ProcessThread classes calling that API throw PNSE so that for iOS/tvOS it's possible to re-use the respective *.UnknownUnix.cs parts.
Tagging subscribers to this area: @dotnet/area-system-diagnostics-process Issue DetailsSince libproc is a private Apple API, it is not available on iOS/tvOS and should be excluded (see #61265 (comment) and above for more details).
|
Tagging subscribers to 'os-ios': @steveisok, @akoeplinger Issue DetailsBackport of #61590 to release/6.0 Customer ImpactA handful of System.Diagnostics.Process API's use libproc when targeting iOS & tvOS. This is a private API and will result in Apple rejecting apps that use it from publishing to the App Store. TestingManual testing RiskLow.
|
…stenerTests on iOS/tvOS (dotnet#61807) This marks System.Diagnostics.TextWriterTraceListenerTests.XmlWriterTraceListenerTests withSkipOnPlatform attribute for iOS/tvOS as those tests try to create a process info, which throws PNSE after S.D.Process API's around libproc have been excluded in dotnet#61590.
This looks like a backport of two PRs: #61590 and #61670, right? Could you please update the description? In #61670, Secondly, was Note: this does not break compilation on a true UnknownUnix (I have tested it in illumos rootfs docker container), so it's not blocking anyone. cc @stephentoub |
yes, it was mostly cosmetic change which, I thought, is unnecessary to backport. It can be easely added back.
yes, as far as I remember, the compatibility analyzer gave me the warnings as those files are included for compiling for iOS/tvOS now. |
Yes, when you add Unsupported attributes, they need to be in all partials. |
The reason why I suggested to use UnknownUnix partial (#61590 (comment)) was to keep the delta smaller (project configuration only without adding new code files). That worked fine and served the purpose. The latter PR #61670 added the attribute usage in UnknownUnix partial, which makes it more confusing for folks porting runtime. We need to remember which SDK supports which version of analyzer and thus the list of platforms, then add annotations. This is an orthogonal to why UnknownUnix were formed. Please decouple the partials if the attribute usage is inevitable (which I don't think is; #61670 is a good-to-have change but YMMV!). |
If re-using UnknownUnix parts for mobile targets breaks their initial semantic, I can duplicate those in iOS-related files. Anyways, the public API's throwing PNSE should have those annotations in order to improve developer experience on mobile platforms. |
Yes, I think decoupling makes sense as the original semantics (dotnet/corefx#4402) for UnknownUnix were to succeed libraries compilation on unofficial platforms. Additional decorators on PNSE throwing APIs for each supported/unsupported platform is not scalable in the context of unknown unix code path. That is the indicator for developer working on porting dotnet runtime to new platforms that there is work to be done in those areas rather than something we will compile as part of the shipping product indefinitely. |
@steveisok could you please send request for tactics approval? |
Closing this as we need to add a little more in main. We'll be able to backport to the maui branch instead. |
Backport of #61590, #61670, #61807, and #61826 to release/6.0
/cc @MaximLipnin @steveisok
Customer Impact
A handful of System.Diagnostics.Process API's use libproc when targeting iOS & tvOS. This is a private API and will result in Apple rejecting apps that use it from publishing to the App Store.
We will PNSE and add
UnsupportedOSPlatform
to the API's that try to access the private API's and disable related tests.Testing
Manual testing
Risk
Low.