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] Reverting: Set AssemblyName.ProcessorArchitecture for compatibility (#81101) #83565

Merged
merged 1 commit into from
Apr 7, 2023

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Mar 17, 2023

This is a revert of #81101

AssemblyName.ProcessorArchitecture is unnatural concept in CoreCLR (and thus the property is deprecated).

The attempt to make the behavior closer to the native implementation in #81101 caused more issues than the original minor compat break that it tried to fix.

Customer Impact

After the original change an application that obtain AssemblyName from assembly files may get ProcessorArchitecture set, which could be now unexpected when the name is subsequently used.

For example a scenario when an app scans assemblies in a folder used to work prior the original "fix" may now fail with:

Loading assembly: LoadAssemblyBug, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null
Loading assembly: Microsoft.AspNetCore.Antiforgery, Version=7.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60
Could not load file or assembly 'Microsoft.AspNetCore.Antiforgery, Version=7.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60, processorArchitecture=AMD64'. The system cannot find the file specified.

See: #83526

We think that the original fix does not precisely replicate the .NET desktop behavior. However, trying to replicate the exact deprecated API behavior seems very difficult. For instance, the API doesn't support ARM64, so we would have to somehow provide back-compat for a deprecated API even as our product evolves in a way that the original API can't support and we don't want to evolve.

There are also similar issues reported directly by internal teams updating to 7.0.201.

Testing

We have regression tests for this feature and they were updated as a part of this change. Our understanding of the "modern" API set is quite good, compared to the deprecated set.

Risk

The change itself has low risk. We are reverting another change.

We also need to acknowledge that the state we are reverting has compat differences with .NET FX and 6.0, which at this point seems preferable.

@ghost
Copy link

ghost commented Mar 17, 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

This is a revert of #81101

AssemblyName.ProcessorArchitecture is unnatural concept in CoreCLR (and thus the property is deprecated).
The attempt to make the behavior closer to the native implementation in #81101 caused more issues than the original compat break that it tried to fix.

See: #83526
There are also issues reported directly by a few internal teams updating to 7.0.201.

TODO: fill the template

Author: VSadov
Assignees: VSadov
Labels:

area-System.Reflection.Metadata

Milestone: -

@jkotas
Copy link
Member

jkotas commented Mar 17, 2023

Are we going to revert it in main as well?

@VSadov
Copy link
Member Author

VSadov commented Mar 17, 2023

Are we going to revert it in main as well?

yes.

the change in main was a bit bigger - not just metadata reader, but also AssemblyName.CoreCLR. Not sure yet which way that should be changed.

@jkotas
Copy link
Member

jkotas commented Mar 17, 2023

AssemblyName.CoreCLR. Not sure yet which way that should be changed.

I think it would be best to make it match System.Reflection.Metadata.

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

Code change LGTM. Please fill out the template

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. we will take for consideration in 7.0.x

@agocke agocke added the Servicing-consider Issue for next servicing release review label Mar 21, 2023
@rbhanda rbhanda added this to the 7.0.6 milestone Mar 23, 2023
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Mar 23, 2023
@VSadov VSadov changed the base branch from release/7.0 to release/7.0-staging March 28, 2023 18:12
Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

The OOB package authoring changes look good.

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-approved Approved for servicing release labels Mar 30, 2023
@carlossanlop
Copy link
Member

Reminder: April 10th is the last day to merge backport PRs to ensure they get included in the May Release. PR owners are now in charge of merging their own PRs.

@carlossanlop
Copy link
Member

I re-targeted this PR to the new release/7.0-staging branch, which is the one that we will use from now on for servicing fixes.

PR owners will now be expected to merge their own servicing PR. The new process is described here: runtime/docs/project/library-servicing.md.

The infra team will be actively monitoring servicing PRs as usual to help with any issues.

@VSadov if you're not waiting for anything else, you can merge anytime, your PR meets all the requirements. Here is the checklist for future reference:

@VSadov
Copy link
Member Author

VSadov commented Apr 7, 2023

@carlossanlop - thanks!! I'll merge it then.

@VSadov VSadov merged commit 7e8c33e into dotnet:release/7.0-staging Apr 7, 2023
@VSadov VSadov deleted the revert81101 branch April 7, 2023 02:46
@ghost ghost locked as resolved and limited conversation to collaborators May 7, 2023
@leecow leecow modified the milestones: 7.0.6, 7.0.7 Jun 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants