Skip to content

Simplify icudat file lookup by specifying ICU_DAT_FILE_PATH as a RuntimeHostConfigurationOption #18914

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.

Sign up for GitHub

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

Merged
ivanpovazan merged 3 commits into from
Sep 8, 2023
Merged

Simplify icudat file lookup by specifying ICU_DAT_FILE_PATH as a RuntimeHostConfigurationOption #18914

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ed26faa
Simplify icudat file lookup by specifying ICU_DAT_FILE_PATH as a Runt…
ivanpovazan Aug 31, 2023
b3c8690
Merge branch 'net8.0' into new-find-icu-data
rolfbjarne Sep 5, 2023
1e06cef
Clean up globalization custom logic
ivanpovazan Sep 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions dotnet/targets/Xamarin.Shared.Sdk.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,11 +165,6 @@
<TargetPlatformSupported Condition=" '$(TargetPlatformIdentifier)' == '$(_PlatformName)' ">true</TargetPlatformSupported>
</PropertyGroup>

<PropertyGroup Condition="'$(_GlobalizationDataFileLocation)' == ''">
<_GlobalizationDataFileLocation Condition="'$(_UseNativeAot)' == 'true'">Resource</_GlobalizationDataFileLocation>
<_GlobalizationDataFileLocation Condition="'$(_UseNativeAot)' != 'true'">Assembly</_GlobalizationDataFileLocation>
</PropertyGroup>

<!-- dotnet publish -->
<Import Project="Xamarin.Shared.Sdk.Publish.targets" />

Expand Down Expand Up @@ -1090,7 +1085,6 @@
<!-- List all the properties passed to xamarin_bridge_vm_initialize in xamarin_vm_initialize (in runtime.m) -->
<_RuntimeConfigReservedProperties Include="APP_PATHS" />
<_RuntimeConfigReservedProperties Include="PINVOKE_OVERRIDE" />
<_RuntimeConfigReservedProperties Include="ICU_DAT_FILE_PATH" />
<_RuntimeConfigReservedProperties Include="TRUSTED_PLATFORM_ASSEMBLIES" />
<_RuntimeConfigReservedProperties Include="NATIVE_DLL_SEARCH_DIRECTORIES" />
<_RuntimeConfigReservedProperties Include="RUNTIME_IDENTIFIER" />
Expand Down Expand Up @@ -1691,7 +1685,7 @@
'%(ResolvedFileToPublish.Filename)%(ResolvedFileToPublish.Extension)' == '$(_GlobalizationDataFile)' And
'%(ResolvedFileToPublish.NuGetPackageId)' == '$(_MonoNugetPackageId)'
"
PublishFolderType="$(_GlobalizationDataFileLocation)"
PublishFolderType="Assembly"
/>

<!-- Remove the libxamarin-*.dylib files we don't want -->
Expand Down
8 changes: 8 additions & 0 deletions msbuild/Xamarin.Shared/Xamarin.Shared.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2870,13 +2870,21 @@ Copyright (C) 2018 Microsoft. All rights reserved.
<!-- needed for GetTargetPath/Build/Rebuild task outputs -->
<_AppExtensionBundlePath>$(MSBuildProjectDirectory)\$(AppBundleDir)</_AppExtensionBundlePath>
</PropertyGroup>
<PropertyGroup>
<_GlobalizationDataFileAppBundleRelativePath Condition="'$(_AppContentsRelativePath)' == ''">$(_GlobalizationDataFile)</_GlobalizationDataFileAppBundleRelativePath>
<_GlobalizationDataFileAppBundleRelativePath Condition="'$(_AppContentsRelativePath)' != ''">$(_AppContentsRelativePath)\$(_GlobalizationDataFile)</_GlobalizationDataFileAppBundleRelativePath>
</PropertyGroup>
<ItemGroup Condition="'$(IsAppExtension)' == 'true'">
<_AppExtensionBundlePath Include="$(MSBuildProjectDirectory)\$(AppBundleDir)">
<!-- We need this metadata to fix the source in VS -->
<BuildSessionId>$(BuildSessionId)</BuildSessionId>
<BuildServerPath>..\..\$(BuildAppName)\$(BuildSessionId)\$(AppBundleDir)</BuildServerPath>
</_AppExtensionBundlePath>
</ItemGroup>
<ItemGroup>
<!-- Replacing backslashes with slashes is required since MSBuild has some issue with resolving backslashes next to a property value: https://github.com/dotnet/msbuild/issues/9200 -->
<RuntimeHostConfigurationOption Include="ICU_DAT_FILE_PATH" Value="$(_GlobalizationDataFileAppBundleRelativePath.Replace('\','/'))" />
Comment thread
emaf marked this conversation as resolved.
</ItemGroup>
</Target>

<Target Name="_CompileProductDefinition" Condition="$(CreatePackage)" DependsOnTargets="_WriteAppManifest;_ComputeTargetArchitectures">
Expand Down
28 changes: 2 additions & 26 deletions runtime/runtime.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,6 @@
#endif
int xamarin_log_level = 0;
const char *xamarin_executable_name = NULL;
#if DOTNET
const char *xamarin_icu_dat_file_name = NULL;
#endif
#if MONOMAC || TARGET_OS_MACCATALYST
NSString * xamarin_custom_bundle_name = @"MonoBundle";
#endif
Expand Down Expand Up @@ -2585,22 +2582,6 @@ -(void) xamarinSetFlags: (enum XamarinGCHandleFlags) flags;
xamarin_vm_initialize ()
{
char *pinvokeOverride = xamarin_strdup_printf ("%p", &xamarin_pinvoke_override);
char *icu_dat_file_path = NULL;
int subtractPropertyCount = 0;

if (xamarin_icu_dat_file_name != NULL && *xamarin_icu_dat_file_name != 0) {
#if !defined (NATIVEAOT)
char path [1024];
if (!xamarin_locate_app_resource (xamarin_icu_dat_file_name, path, sizeof (path))) {
LOG (PRODUCT ": Could not locate the ICU data file '%s' in the app bundle.\n", xamarin_icu_dat_file_name);
} else {
icu_dat_file_path = strdup (path);
}
#endif // !defined (NATIVEAOT)
} else {
subtractPropertyCount++;
}

char *trusted_platform_assemblies = xamarin_compute_trusted_platform_assemblies ();
char *native_dll_search_directories = xamarin_compute_native_dll_search_directories ();

Expand All @@ -2613,7 +2594,6 @@ -(void) xamarinSetFlags: (enum XamarinGCHandleFlags) flags;
"TRUSTED_PLATFORM_ASSEMBLIES",
"NATIVE_DLL_SEARCH_DIRECTORIES",
"RUNTIME_IDENTIFIER",
"ICU_DAT_FILE_PATH", // Must be last.
};
const char *propertyValues[] = {
xamarin_get_bundle_path (),
Expand All @@ -2622,20 +2602,16 @@ -(void) xamarinSetFlags: (enum XamarinGCHandleFlags) flags;
trusted_platform_assemblies,
native_dll_search_directories,
RUNTIMEIDENTIFIER,
icu_dat_file_path, // might be NULL, if so we say we're passing one property less that what we really are (to skip this last one). This also means that this property must be the last one
};
static_assert (sizeof (propertyKeys) == sizeof (propertyValues), "The number of keys and values must be the same.");

int propertyCount = (int) (sizeof (propertyValues) / sizeof (propertyValues [0])) - subtractPropertyCount;
int propertyCount = (int) (sizeof (propertyValues) / sizeof (propertyValues [0]));
bool rv = xamarin_bridge_vm_initialize (propertyCount, propertyKeys, propertyValues);
xamarin_free (pinvokeOverride);

xamarin_free (pinvokeOverride);
xamarin_free (trusted_platform_assemblies);
xamarin_free (native_dll_search_directories);

if (icu_dat_file_path != NULL)
free (icu_dat_file_path);

if (!rv)
xamarin_assertion_message ("Failed to initialize the VM");
}
Expand Down
3 changes: 0 additions & 3 deletions tools/dotnet-linker/Steps/GenerateMainStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);");
Copy link
Copy Markdown
Member

@akoeplinger akoeplinger Sep 6, 2023

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?

Copy link
Copy Markdown
Member

@rolfbjarne rolfbjarne Sep 6, 2023

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.

Copy link
Copy Markdown
Member

@rolfbjarne rolfbjarne Sep 6, 2023

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:

3f6d43c

so maybe @mkhamoyan knows if these environment variables are really needed or not?

Copy link
Copy Markdown
Contributor

@mkhamoyan mkhamoyan Sep 6, 2023

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.

Copy link
Copy Markdown
Member Author

@ivanpovazan ivanpovazan Sep 6, 2023

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?

Copy link
Copy Markdown
Member

@akoeplinger akoeplinger Sep 6, 2023 •
edited
Loading

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

Copy link
Copy Markdown
Member Author

@ivanpovazan ivanpovazan Sep 6, 2023

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"
    }

Copy link
Copy Markdown
Member Author

@ivanpovazan ivanpovazan Sep 6, 2023

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

} else {
if (Configuration.HybridGlobalization)
contents.AppendLine ("\tsetenv (\"DOTNET_SYSTEM_GLOBALIZATION_HYBRID\", \"1\", 1);");
contents.AppendLine ($"\txamarin_icu_dat_file_name = \"{Configuration.GlobalizationDataFile}\";");
}
if (Configuration.Application.PackageManagedDebugSymbols && Configuration.Application.UseInterpreter)
contents.AppendLine ($"\tsetenv (\"DOTNET_MODIFIABLE_ASSEMBLIES\", \"debug\", 1);");
Expand Down