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

Implement resource lookup in satellite assemblies #86689

Merged
merged 4 commits into from
May 29, 2023

Conversation

MichalStrehovsky
Copy link
Member

Fixes #86651.

This was implemented in .NET Native so we just need to resurface it from the compiler.

Note that this is not full support for satellite assemblies (in the assembly binder, etc.) that never existed and nobody ever asked for and I don't even know what it entails.

Cc @dotnet/ilc-contrib

@ghost
Copy link

ghost commented May 24, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #86651.

This was implemented in .NET Native so we just need to resurface it from the compiler.

Note that this is not full support for satellite assemblies (in the assembly binder, etc.) that never existed and nobody ever asked for and I don't even know what it entails.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@vitek-karas
Copy link
Member

I think we should add at least some testing for this.

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member Author

I think we should add at least some testing for this.

You made me look harder for an existing test because I'm not keen to learn how to make satellite assemblies. Hopefully it will pass.

@vitek-karas
Copy link
Member

I'm not keen to learn how to make satellite assemblies.

Not sure if it's a good idea in runtime repo, but I would use Humanizer - it's a prime example of a NuGet which uses localized resources, so if that doesn't work, it's broken.

@MichalStrehovsky
Copy link
Member Author

MichalStrehovsky commented May 24, 2023

I'm not keen to learn how to make satellite assemblies.

Not sure if it's a good idea in runtime repo, but I would use Humanizer - it's a prime example of a NuGet which uses localized resources, so if that doesn't work, it's broken.

I used System.CommandLine locally. It ships a ton of them. They all have the same strings because everything around System.CommandLine has been a disappointment for me and why would this be any different. Hacked my NuGet cache and updated a string in one of them with a hex editor so I could test this. Still better than learning how to make them.

@MichalStrehovsky MichalStrehovsky marked this pull request as draft May 24, 2023 10:48
@MichalStrehovsky
Copy link
Member Author

Back to draft. Found some issues when the resources don't come from nuget. We might need a brand new command line argument for these things.

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky MichalStrehovsky marked this pull request as ready for review May 25, 2023 05:30
@MichalStrehovsky
Copy link
Member Author

This is ready for review now.

@MichalStrehovsky
Copy link
Member Author

Wow, what a dumb reason to fail the build!

2023-05-25T06:12:16.9819457Z     Unhandled Exception: System.Globalization.CultureNotFoundException: Only the invariant culture is supported in globalization-invariant mode. See https://aka.ms/GlobalizationInvariantMode for more information. (Parameter 'name')
2023-05-25T06:12:16.9826571Z     fr-FR is an invalid culture identifier.
2023-05-25T06:12:16.9833329Z        at System.Reflection.AssemblyName.set_CultureName(String) + 0xa4
2023-05-25T06:12:16.9839851Z        at Internal.TypeSystem.Ecma.EcmaAssembly.GetName() + 0x325
2023-05-25T06:12:16.9846426Z        at ILCompiler.UsageBasedMetadataManager.<GetSatelliteAssemblies>d__25.MoveNext() + 0x42

@MichalStrehovsky
Copy link
Member Author

Fixes #86651.

This was implemented in .NET Native so we just need to resurface it from the compiler.

Note that this is not full support for satellite assemblies (in the assembly binder, etc.) that never existed and nobody ever asked for and I don't even know what it entails.

Cc @dotnet/ilc-contrib

@MichalStrehovsky MichalStrehovsky merged commit b134fa2 into dotnet:main May 29, 2023
@MichalStrehovsky MichalStrehovsky deleted the satellite branch May 29, 2023 23:58
@ghost ghost locked as resolved and limited conversation to collaborators Jun 29, 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.

Satellite assembly support in NativeAOT
3 participants