-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Conversation
Tagging subscribers to this area: @dotnet/area-system-reflection-metadata Issue DetailsThis is a revert of #81101
See: #83526 TODO: fill the template
|
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. |
I think it would be best to make it match System.Reflection.Metadata. |
There was a problem hiding this 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
There was a problem hiding this 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
There was a problem hiding this 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.
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. |
I re-targeted this PR to the new 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:
|
@carlossanlop - thanks!! I'll merge it then. |
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:
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.