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

Reverting: Set AssemblyName.ProcessorArchitecture for compatibility (#80581) #84028

Merged
merged 3 commits into from
Apr 1, 2023

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Mar 28, 2023

This is a revert of #80581

Also see: [release/7.0] Reverting: Set AssemblyName.ProcessorArchitecture for compatibility (#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 #80581 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.

@ghost
Copy link

ghost commented Mar 28, 2023

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

Issue Details

This is a revert of #80581

Also see: [release/7.0] Reverting: Set AssemblyName.ProcessorArchitecture for compatibility (#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 #80581 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.

Author: VSadov
Assignees: VSadov
Labels:

area-System.Reflection

Milestone: -

#pragma warning restore SYSLIB0037
}

private static ProcessorArchitecture CalculateProcArch(PortableExecutableKinds pek, ImageFileMachine ifm, AssemblyNameFlags aFlags)
private static ProcessorArchitecture CalculateProcArchIndex(PortableExecutableKinds pek, ImageFileMachine ifm, AssemblyNameFlags flags)
{
Copy link
Member

Choose a reason for hiding this comment

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

I think we can keep this part as is. It is simpler, it has better comments, and it is functional equivalent to the old code.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, we would delete this and just return ProcessorArchitecture.None to be in sync with the S.R.M implementation, It is probably not worth it to spend time on pushing the breaking change through - we can keep these ~20 lines for compat.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, we would delete this and just return ProcessorArchitecture.None to be in sync with the S.R.M

Actually, since we are breaking this one way or another, maybe we should break this all the way and just return None ?
There is some time in 8.0 to see if this gets anyone in trouble.

@VSadov
Copy link
Member Author

VSadov commented Apr 1, 2023

the wasm failure is #83655

@VSadov
Copy link
Member Author

VSadov commented Apr 1, 2023

Thanks!

@VSadov VSadov merged commit eeb49c4 into dotnet:main Apr 1, 2023
@VSadov VSadov deleted the revPArch branch April 1, 2023 01:48
@ghost ghost locked as resolved and limited conversation to collaborators May 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants