Skip to content
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

Conversation

MaximLipnin
Copy link
Contributor

@MaximLipnin MaximLipnin commented Nov 16, 2021

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.

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.
@ghost
Copy link

ghost commented Nov 16, 2021

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

Issue Details

Since 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).
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.

Author: MaximLipnin
Assignees: -
Labels:

area-System.Diagnostics.Process

Milestone: -

@steveisok steveisok added the Servicing-consider Issue for next servicing release review label Nov 16, 2021
@danmoseley danmoseley added the os-ios Apple iOS label Nov 18, 2021
@ghost
Copy link

ghost commented Nov 18, 2021

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #61590 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.

Testing

Manual testing

Risk

Low.

Author: MaximLipnin
Assignees: -
Labels:

Servicing-consider, area-System.Diagnostics.Process, os-ios

Milestone: -

…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.
@am11
Copy link
Member

am11 commented Nov 19, 2021

This looks like a backport of two PRs: #61590 and #61670, right? Could you please update the description?

In #61670, <NoWarn..> suppression was deleted from project file and inlined on struct definition. Here you have both suppressions, is it intentional?

Secondly, was UnsupportedOSPlatformAttribute usage in UnknownUnix files needed? UnknownUnix is like a default implementation: "all $other unix-like OSes where dotnet/runtime is not yet fully ported" (illumos/Solaris/Haiku/FireflyBSD ... and the list goes on..), while UnsupportedOSPlatformAttribute requires hardcoded platform name "ignore on this specific platform and I know the name which one". They are competing concepts.

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

@MaximLipnin
Copy link
Contributor Author

In #61670, <NoWarn..> suppression was deleted from project file and inlined on struct definition. Here you have both suppressions, is it intentional?

yes, it was mostly cosmetic change which, I thought, is unnecessary to backport. It can be easely added back.

Secondly, was UnsupportedOSPlatformAttribute usage in UnknownUnix files needed?

yes, as far as I remember, the compatibility analyzer gave me the warnings as those files are included for compiling for iOS/tvOS now.

@steveisok
Copy link
Member

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.

@am11
Copy link
Member

am11 commented Nov 19, 2021

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!).

@MaximLipnin
Copy link
Contributor Author

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.

@am11
Copy link
Member

am11 commented Nov 19, 2021

If re-using UnknownUnix parts for mobile targets breaks their initial semantic, I can duplicate those in iOS-related files.

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.

@marek-safar
Copy link
Contributor

@steveisok could you please send request for tactics approval?

@marek-safar marek-safar changed the title Exclude the managed code around libproc on iOS/tvOS (#61590) [release/6.0] Exclude the managed code around libproc on iOS/tvOS (#61590) Nov 22, 2021
@steveisok
Copy link
Member

Closing this as we need to add a little more in main. We'll be able to backport to the maui branch instead.

@steveisok steveisok closed this Nov 23, 2021
@MaximLipnin MaximLipnin deleted the backport-pr61590-to-release-6.0 branch November 23, 2021 16:28
@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Diagnostics.Process os-ios Apple iOS Servicing-consider Issue for next servicing release review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants