-
Notifications
You must be signed in to change notification settings - Fork 516
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
Simplify icudat file lookup by specifying ICU_DAT_FILE_PATH as a RuntimeHostConfigurationOption #18914
Conversation
…imeHostConfigurationOption
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There seems to be an issue with generator tests in Windows integration jobs, where it fails with:
although it doesn't seem to be related to this change. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@@ -31,16 +31,13 @@ protected override void TryEndProcess () | |||
|
|||
contents.AppendLine ("#include <stdlib.h>"); | |||
contents.AppendLine (); | |||
contents.AppendLine ("extern \"C\" const char *xamarin_icu_dat_file_name;"); | |||
contents.AppendLine (); | |||
contents.AppendLine ("static void xamarin_initialize_dotnet ()"); | |||
contents.AppendLine ("{"); | |||
if (Configuration.InvariantGlobalization) { | |||
contents.AppendLine ("\tsetenv (\"DOTNET_SYSTEM_GLOBALIZATION_INVARIANT\", \"1\", 1);"); |
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.
for a future PR: this could be a runtime config option too: System.Globalization.Invariant
(https://learn.microsoft.com/en-us/dotnet/core/runtime-config/globalization)
or actually we might not even need to do anything if the InvariantGlobalization
msbuild property is set it should get turned into the runtime config automatically?
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.
Yes, this is from the early .NET 5 days, so the runtime config option probably didn't work then.
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 DOTNET_SYSTEM_GLOBALIZATION_HYBRID
environment variable is pretty new though:
so maybe @mkhamoyan knows if these environment variables are really needed or not?
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.
I added DOTNET_SYSTEM_GLOBALIZATION_HYBRID
following to the logic done for Invariant mode. Not sure if these env variables are really needed or not.
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 runtime seems to first look into the runtime config switches and then as a fallback into env variables:
https://github.com/dotnet/runtime/blob/1487d46e4ff7731b52af92b1228373a2a66ed6bf/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs#L15-L17
which calls into: https://github.com/dotnet/runtime/blob/1487d46e4ff7731b52af92b1228373a2a66ed6bf/src/libraries/System.Private.CoreLib/src/System/AppContextConfigHelper.cs#L13-L22
Do you want me to remove setting up env variables explicitly in xamarin_initialize_dotnet
?
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.
Do you want me to remove setting up env variables explicitly in xamarin_initialize_dotnet?
we can do that in a separate PR after verifying that the switches do end up in runtimeconfig.bin etc. but yeah, it'd be nice to reduce custom logic
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.
I checked this by building MySingleView
and inspecting MySingleView.runtimeconfig.json
:
"configProperties": {
"Microsoft.Extensions.DependencyInjection.VerifyOpenGenericServiceTrimmability": true,
"System.AggressiveAttributeTrimming": false,
"System.ComponentModel.TypeConverter.EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization": false,
"System.Diagnostics.Debugger.IsSupported": true,
"System.Diagnostics.Tracing.EventSource.IsSupported": false,
"System.Globalization.Invariant": false,
"System.Globalization.Hybrid": false,
"System.Net.Http.EnableActivityPropagation": false,
"System.Net.Http.UseNativeHttpHandler": true,
"System.Reflection.NullabilityInfoContext.IsSupported": false,
"System.Resources.ResourceManager.AllowCustomResourceTypes": false,
"System.Resources.UseSystemResourceKeys": false,
"System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported": false,
"System.Runtime.InteropServices.BuiltInComInterop.IsSupported": false,
"System.Runtime.InteropServices.EnableConsumingManagedCodeFromNativeHosting": false,
"System.Runtime.InteropServices.EnableCppCLIHostActivation": false,
"System.Runtime.InteropServices.Marshalling.EnableGeneratedComInterfaceComImportInterop": false,
"System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization": false,
"System.StartupHookProvider.IsSupported": false,
"System.Text.Encoding.EnableUnsafeUTF7Encoding": false,
"System.Text.Json.JsonSerializer.IsReflectionEnabledByDefault": true,
"System.Threading.Thread.EnableAutoreleasePool": true,
"System.Linq.Expressions.CanEmitObjectArrayDelegate": false,
"ICU_DAT_FILE_PATH": "icudt.dat"
}
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.
I have included the clean up in this PR, will see what the tests say
✅ API diff for current PR / commitLegacy Xamarin (No breaking changes)
NET (empty diffs)
✅ API diff vs stableLegacy Xamarin (No breaking changes).NET (No breaking changes)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
🔥 [PR Build] Build failed 🔥Build failed for the job 'Build macOS tests' Pipeline on Agent |
💻 [CI Build] Windows Integration Tests passed 💻✅ All Windows Integration Tests passed. Pipeline on Agent |
📚 [PR Build] Artifacts 📚Packages generatedView packagesPipeline on Agent |
💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻✅ All tests on macOS M1 - Mac Big Sur (11.5) passed. Pipeline on Agent |
💻 [PR Build] Tests on macOS M1 - Mac Ventura (13.0) passed 💻✅ All tests on macOS M1 - Mac Ventura (13.0) passed. Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
🚀 [CI Build] Test results 🚀Test results✅ All tests passed on VSTS: simulator tests. 🎉 All 233 tests passed 🎉 Tests counts✅ bcl: All 69 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
/cc @directhex fyi |
Fixes #18471