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/7.0] Set AssemblyName.ProcessorArchitecture for compatibility. #81101

Merged
merged 5 commits into from
Feb 8, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jan 24, 2023

Fixes: #77697

Partial backport of #80581 to release/7.0
Backport of #80878 (test fix)

Includes only metadata reader fixes. The consistency changes to AssemblyName.CoreCLR are not ported to reduce risk of incompatibilities in a servicing release.

Customer Impact

Moving assembly name parsing to unified managed implementation unintentionally omitted computing ProcessorArchitecture.
Even though the ProcessorArchitecture is a deprecated API, it was a breaking change in some rare scenarios.

This change brings back a simplified form of computing ProcessorArchitecture.

"Simplified" here is that we do not handle completely broken/not-loadable assemblies. In those cases we return default value. This should be sufficient to satisfy the scenarios that still use this, otherwise deprecated, API.

Testing

A test was added and is backported together with this change.

Risk

Low. We are reintroducing an implementation of an API instead of always returning a default value.

@ghost
Copy link

ghost commented Jan 24, 2023

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

Issue Details

Backport of #80581 to release/7.0

/cc @VSadov

Customer Impact

Testing

Risk

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Reflection.Metadata

Milestone: -

@VSadov VSadov requested a review from jkotas January 24, 2023 18:16
@jkotas
Copy link
Member

jkotas commented Jan 24, 2023

This is touching System.Reflection.Metadata that also ships as independent nuget package. I think you need include packaging authoring change as part of the PR to make sure that we ship a new version of System.Reflection.Metadata package.

@VSadov
Copy link
Member

VSadov commented Jan 24, 2023

I think you need include packaging authoring change as part of the PR

Are there examples how this is done? We probably did this before.

@jkotas
Copy link
Member

jkotas commented Jan 24, 2023

https://github.com/dotnet/runtime/blob/main/docs/project/library-servicing.md

@jkotas
Copy link
Member

jkotas commented Jan 24, 2023

https://github.com/dotnet/runtime/search?q=library-servicing.md&type=issues shows examples where this was done before

@VSadov VSadov added the Servicing-consider Issue for next servicing release review label Jan 25, 2023
@ericstj
Copy link
Member

ericstj commented Jan 26, 2023

Do we think this might reintroduce warnings in MSBuild in servicing? While that might be desirable, we should probably call that out as a risk. cc @rainersigwald

@rainersigwald
Copy link
Member

Do we think this might reintroduce warnings in MSBuild in servicing? While that might be desirable, we should probably call that out as a risk. cc @rainersigwald

This is a good concern but I think it's ok.

This will reactivate the longstanding MSBuild codepath that covers the non-ProcessorArchitecture.None scenario, so it could raise an error that running on the 7.0.0 runtime wouldn't.

Mitigations:

  1. It was like this for previous releases.
  2. It is still like this for .NET Framework MSBuild (i.e. Visual Studio-driven builds).
  3. There is a property that can be set to avoid the warning/error (<ResolveAssemblyWarnOrErrorOnTargetArchitectureMismatch>none</ResolveAssemblyWarnOrErrorOnTargetArchitectureMismatch>)
  4. I think it's generally desired behavior.

@ericstj
Copy link
Member

ericstj commented Jan 26, 2023

Got it, do you think we should up the risk to "medium" and plan for documentation (release notes) that warn folks about this?

@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 26, 2023
@rbhanda rbhanda added this to the 7.0.4 milestone Jan 26, 2023
@ericstj ericstj added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jan 26, 2023
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jan 26, 2023
@ghost
Copy link

ghost commented Jan 26, 2023

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@rainersigwald
Copy link
Member

rainersigwald commented Jan 26, 2023

Per tactics we need a relnote. It should ideally link to https://learn.microsoft.com/visualstudio/msbuild/errors/msb3270, which has docs on the error and property.

@ericstj
Copy link
Member

ericstj commented Jan 26, 2023

Conclusion from tactics was "yes" to my question above. Let's file the breaking change doc for this - warning folks that the fix will bring back the warning. We should tag @rbhanda in that breaking change issue to ensure the details get added to servicing release notes.

@VSadov
Copy link
Member

VSadov commented Jan 28, 2023

Conclusion from tactics was "yes" to my question above. Let's file the breaking change doc for this - warning folks that the fix will bring back the warning. We should tag @rbhanda in that breaking change issue to ensure the details get added to servicing release notes.

Just to make sure - do I need to do the above or it will be someone who better understands what the msbuild issue is about, or the committer of this change?

@carlossanlop
Copy link
Member

carlossanlop commented Feb 8, 2023

Approved by Tactics for 7.0.4.
Signed off by area owners.
Required OOB package authoring changes look good.
CI failures unrelated: #81544
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit b384ed7 into release/7.0 Feb 8, 2023
@carlossanlop carlossanlop deleted the backport/pr-80581-to-release/7.0 branch February 8, 2023 23:57
@ghost ghost locked as resolved and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection.Metadata breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants