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/9.0-preview7] Fix platform analyzer attribute order for MacCatalyst #105410

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jul 24, 2024

Backport of #105409 to release/9.0-preview7

/cc @akoeplinger

Customer Impact

  • Customer reported
  • Found internally

We need to make sure the attribute for MacCatalyst comes after the iOS one due to how MacCatalyst is a superset of iOS: learn.microsoft.com/en-us/dotnet/standard/analyzers/platform-compat-analyzer#platform-inclusion

This caused an error in aspnetcore in the latest dependency flow because the analyzer thought AesGcm is only supported on MacCatalyst:

error CA1416: (NETCORE_ENGINEERING_TELEMETRY=Build) This call site is reachable on all platforms. 'AesGcm.Decrypt(ReadOnlySpan, ReadOnlySpan, ReadOnlySpan, Span, ReadOnlySpan)' is only supported on: 'maccatalyst' 13.0 and later.

Regression

  • Yes
  • No

The attributes were only added recently a couple days ago.

Testing

Tested locally by overwriting the ref assembly in my dotnet SDK and observing the analyzer warning to be fixed.

Risk

Low, this just reorders the attributes on two classes in ref assemblies (the rest of the changes are internal in implementation assemblies and no-op).

We need to make sure the attribute for MacCatalyst comes _after_ the iOS one due to how MacCatalyst is a superset of iOS: https://learn.microsoft.com/en-us/dotnet/standard/analyzers/platform-compat-analyzer#platform-inclusion

This caused an error in aspnetcore in the latest dependency flow because the analyzer thought AesGcm is _only_ supported on MacCatalyst:
> error CA1416: (NETCORE_ENGINEERING_TELEMETRY=Build) This call site is reachable on all platforms. 'AesGcm.Decrypt(ReadOnlySpan<byte>, ReadOnlySpan<byte>, ReadOnlySpan<byte>, Span<byte>, ReadOnlySpan<byte>)' is only supported on: 'maccatalyst' 13.0 and later.
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

@carlossanlop
Copy link
Member

Please send the email to Tactics requesting approval, we're running short on time for merging backports to prev7.

@akoeplinger akoeplinger added Servicing-consider Issue for next servicing release review and removed new-api-needs-documentation labels Jul 24, 2024
@ericstj
Copy link
Member

ericstj commented Jul 24, 2024

We need to make sure the attribute for MacCatalyst comes after the iOS one due to how MacCatalyst is a superset of iOS:

That's really unusual. Should we fix the analyzer? I don't think it's normal to have attribute order matter.

This fix is fine to unblock P7 but I'm surprised to learn about this.

Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

This unblocks p7 codeflow even if the actual problem is in the analyzer

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jul 24, 2024
@carlossanlop
Copy link
Member

Approved by Tactics via email. Eric said offline his comment is non-blocking. Merging now.

@carlossanlop
Copy link
Member

There was an unrelated test failure in CodePages. The tests succeeded but the test execution was marked as failed for some weird reason. Anyway, not a problem.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants