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.

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

Jump to bottom

Mark System.Console APIs as unsupported on Android #50931

Merged
merged 10 commits into from
Apr 20, 2021
Merged

Mark System.Console APIs as unsupported on Android #50931

merged 10 commits into from
Apr 20, 2021

Conversation

MaximLipnin
Copy link
Contributor

@MaximLipnin MaximLipnin commented Apr 8, 2021 •
edited
Loading

Annotated public System.Console APIs throwing PNSE on Android.

No changes added to ref-part b/c no warnings appeared (maybe I missed something?).

Part of #47911.

@ghost
Copy link

ghost commented Apr 8, 2021

Tagging subscribers to this area: @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

Annotated public System.Console APIs throwing PNSE on Android.

No changes added to ref-part b/c no warnings appeared (maybe I missed something?).

Part of #47911.

Author: MaximLipnin
Assignees: -
Labels:

area-System.Console
Milestone: -

This was referenced Apr 8, 2021
Closed
Closed
@stephentoub
Copy link
Member

No changes added to ref-part b/c no warnings appeared (maybe I missed something?).

You need to add these to the ref for them to be meaningful to anyone outside of System.Console. The purpose of the attributes is to give warnings to consumers of the API that they might be calling something that doesn't work.

@MaximLipnin
Copy link
Contributor Author

No changes added to ref-part b/c no warnings appeared (maybe I missed something?).

You need to add these to the ref for them to be meaningful to anyone outside of System.Console. The purpose of the attributes is to give warnings to consumers of the API that they might be calling something that doesn't work.

Yes, but as far as I remember there should be the analyzer warnings saying that one is missing the attribute annotations in the respective ref part. I didn't get such a warning so that's why I left the comment about ref part. Actually no issue to add it as well.

@MaximLipnin MaximLipnin force-pushed the mark_console_apis_unsupported_on_android branch from 5d87cad to ef531c0 Compare April 9, 2021 09:32
@carlossanlop
Copy link
Member

Yes, but as far as I remember there should be the analyzer warnings saying that one is missing the attribute annotations in the respective ref part.

@stephentoub @buyaa-n is this a bug in the analyzer?

@MaximLipnin MaximLipnin force-pushed the mark_console_apis_unsupported_on_android branch from 3755cf4 to 14f0cee Compare April 12, 2021 07:24
@buyaa-n
Copy link
Contributor

buyaa-n commented Apr 12, 2021

Yes, but as far as I remember there should be the analyzer warnings saying that one is missing the attribute annotations in the respective ref part.

@stephentoub @buyaa-n is this a bug in the analyzer?

Not sure if that is a bug in ApiCompat analyzer CC @safern

@MaximLipnin MaximLipnin force-pushed the mark_console_apis_unsupported_on_android branch from 14f0cee to 2a87a7a Compare April 13, 2021 11:24
@MaximLipnin MaximLipnin requested a review from marek-safar April 15, 2021 07:47
@MaximLipnin
Copy link
Contributor Author

Could anyone please give another review to it?

marek-safar
marek-safar reviewed Apr 15, 2021
View reviewed changes
src/libraries/Microsoft.Extensions.Logging.Console/src/ConsoleLoggerProvider.cs Outdated Show resolved Hide resolved
...braries/Microsoft.Extensions.Logging.Console/tests/TrimmingTests/AddConsoleFormatterTests.cs Outdated Show resolved Hide resolved
src/libraries/tests.proj Outdated
@@ -23,6 +23,8 @@
<ItemGroup Condition="'$(TargetOS)' == 'Android'">
<!-- Never going to run on Android -->
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Security.Cryptography.OpenSsl\tests\System.Security.Cryptography.OpenSsl.Tests.csproj" />

<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.Extensions.Logging.Console\tests\TrimmingTests\Microsoft.Extensions.Logging.Console.TrimmingTests.proj" />
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be better to fix the test

@MaximLipnin MaximLipnin force-pushed the mark_console_apis_unsupported_on_android branch from 2a87a7a to c052bd9 Compare April 15, 2021 13:41
@eerhardt
Copy link
Member 

    [UnsupportedOSPlatform("browser")]

Why isn't CursorLeft and CursorTop annotated for Android?

Refers to: src/libraries/System.Console/src/System/Console.cs:423 in 54c4907. [](commit_id = 54c4907b4b45d63bcb8527db09cbd717434e19c1, deletion_comment = False)

@eerhardt
Copy link
Member 

    [UnsupportedOSPlatform("browser")]

Why is OpenStandardInput() annotated as unsupported, but OpenStandardInput(int bufferSize) isn't?

Refers to: src/libraries/System.Console/src/System/Console.cs:555 in 54c4907. [](commit_id = 54c4907b4b45d63bcb8527db09cbd717434e19c1, deletion_comment = False)

@MaximLipnin MaximLipnin force-pushed the mark_console_apis_unsupported_on_android branch from 54c4907 to 8053477 Compare April 16, 2021 12:40
@MaximLipnin MaximLipnin force-pushed the mark_console_apis_unsupported_on_android branch from 8053477 to e8927bf Compare April 19, 2021 07:18
@runfoapp runfoapp bot mentioned this pull request Apr 19, 2021
Closed
@MaximLipnin
Copy link
Contributor Author

Please, give it another review round 😄

jeffhandley
jeffhandley approved these changes Apr 20, 2021
View reviewed changes
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

Looks good to me, but please also wait for a re-review from one of the previous reviewers.

@MaximLipnin MaximLipnin merged commit f9ce10f into dotnet:main Apr 20, 2021
@MaximLipnin MaximLipnin deleted the mark_console_apis_unsupported_on_android branch April 20, 2021 10:14
@adamsitnik adamsitnik mentioned this pull request May 6, 2021
Open
@ghost ghost locked as resolved and limited conversation to collaborators May 20, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@buyaa-n buyaa-n buyaa-n left review comments

@eerhardt eerhardt eerhardt left review comments

@jeffhandley jeffhandley jeffhandley approved these changes

@marek-safar marek-safar marek-safar approved these changes

@directhex directhex Awaiting requested review from directhex

@steveisok steveisok Awaiting requested review from steveisok

@akoeplinger akoeplinger Awaiting requested review from akoeplinger

@mdh1418 mdh1418 Awaiting requested review from mdh1418

Assignees
No one assigned
Labels
area-System.Console os-android
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
8 participants
@MaximLipnin @stephentoub @carlossanlop @buyaa-n @eerhardt @marek-safar @jeffhandley @karelz