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

Add local build for System.Globalization shim #72896

Merged
merged 28 commits into from
Sep 6, 2022
Merged

Conversation

smhmhmd
Copy link
Contributor

@smhmhmd smhmhmd commented Jul 26, 2022

Bundle the System.Globalization.Native shim sources into the native AOT toolchain package and compile them automatically using C/C++ compiler on the target machine.

Compiling of the shim sources using C/C++ compiler is done during dotnet publish as follows:
dotnet publish /p:LocalSystemGlobalizationNative=1 -bl -r linux-x64

This change is 'opt-in', it is enabled only by using /p:LocalSystemGlobalizationNative=1

Bundle the System.Globalization.Native shim sources into the native AOT toolchain package and compile them automatically using C/C++ compiler on the target machine.

Compiling of the shim sources using C/C++ compiler is done during dotnet publish as follows:
  dotnet publish  /p:LocalSystemGlobalizationNative=1 -bl -r linux-x64

This change is 'opt-in', it is enabled only by using /p:LocalSystemGlobalizationNative=1
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 26, 2022
@dnfadmin
Copy link

dnfadmin commented Jul 26, 2022

CLA assistant check
All CLA requirements met.

@ghost
Copy link

ghost commented Jul 26, 2022

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

Issue Details

Bundle the System.Globalization.Native shim sources into the native AOT toolchain package and compile them automatically using C/C++ compiler on the target machine.

Compiling of the shim sources using C/C++ compiler is done during dotnet publish as follows:
dotnet publish /p:LocalSystemGlobalizationNative=1 -bl -r linux-x64

This change is 'opt-in', it is enabled only by using /p:LocalSystemGlobalizationNative=1

Author: smhmhmd
Assignees: -
Labels:

area-System.Globalization

Milestone: -

@tarekgh
Copy link
Member

tarekgh commented Jul 26, 2022

CC @jkotas

@tarekgh
Copy link
Member

tarekgh commented Jul 26, 2022

#70848

@am11
Copy link
Member

am11 commented Jul 27, 2022

So far we have not required cmake dependency for PublishAot=tru on the user machine, I think it would be nice to keep it that way.

@jkotas
Copy link
Member

jkotas commented Jul 27, 2022

So far we have not required cmake dependency for PublishAot=tru on the user machine, I think it would be nice to keep it that way

I do not think that cmake dependency is a problem for this special mode of NativeAOT publishing. This mode can grow into optionally building all C/C++ that the runtime is composed from.

@am11
Copy link
Member

am11 commented Jul 27, 2022

We can construct the same arguments in our existing msbuild targets which invokes the compiler, without requiring a fully fledged build system from user. It just feels heavy (even for this optional mode) as there is no system introspection going on only a few paths and macro definitions which can be passed with rest of the command line arguments.

* Copy shim source code into users Obj directory at IntermediateOutputPath and compile
** $ ls -al obj/Debug/net7.0/linux-x64/shim_source_code/libs/System.Globalization.Native/build/libSystem.Globalization.Native-Static.a

* Add shell script for readability

* Add copyright headers
Add change that was part of previous commit
@jkotas
Copy link
Member

jkotas commented Aug 2, 2022

This should not be needed. The existing STATIC_ICU has the same effect - just need to define it when building.

Addressed in next revision

This does not seem to be addressed. pal_icushim_native_static.c file is still there.

* Include existing pal_icushim_static.c by using STATIC_ICU define
* Use IlcSdkPath to copy shim source code
@smhmhmd
Copy link
Contributor Author

smhmhmd commented Aug 2, 2022

This should not be needed. The existing STATIC_ICU has the same effect - just need to define it when building.

Addressed in next revision

This does not seem to be addressed. pal_icushim_native_static.c file is still there.

Removed pal_icushim_native_static.c in latest revision

…pile defs

* Remove IlcHostPackagePath check
* StaticICULinking property is true/false
* Remove -g and -fPIC from definitions
* Use minimal warning level
* Remove -DHOST_AMD64
* Remove -O0
* Remove -fms-extensions -fwrapv
* Standardize on "local build"
* Rename build-nativeaot-shim-standalone.sh to build.sh
* Script name in the error message should match the actual script name
* Rename source_code to native/src

Test code:
$ cat Program.cs
using System.Globalization;

namespace CurrentCulture
{
	class Program
	{
		static void Main(string[] args)
		{
			double val = 1235.56;

			Console.WriteLine($"Current culture: {CultureInfo.CurrentCulture.Name}");
			Console.WriteLine(val);

			CultureInfo.DefaultThreadCurrentCulture = new CultureInfo("en-US");

			Console.WriteLine($"Current culture: {CultureInfo.CurrentCulture.Name}");
			Console.WriteLine(val);
		}
	}
}

$ cat hello-4.csproj
<Project Sdk="Microsoft.NET.Sdk">
 <PropertyGroup>
  <OutputType>Exe</OutputType>
  <TargetFramework>net7.0</TargetFramework>
  <RootNamespace>hello_4</RootNamespace>
  <ImplicitUsings>enable</ImplicitUsings>
  <Nullable>enable</Nullable>
 </PropertyGroup>

$ cat nuget.config
<?xml version="1.0" encoding="utf-8"?>
<configuration>
 <packageSources>
  <!--To inherit the global NuGet package sources remove the <clear/> line below -->
  <clear />
    <add key="repositoryPath" value="/home/ubuntu/runtime/artifacts/packages/Debug/Shipping" />
    <add key="dotnet7" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet7/nuget/v3/index.json" />
 </packageSources>
</configuration>

Test code build:
rm ./bin/Debug/net7.0/linux-arm64/native/hello-4
~/dotnet-sdk/dotnet clean
~/dotnet-sdk/dotnet publish /p:PublishAot=true /p:StaticICULinking=true -bl -r linux-arm64 --self-contained -v d 2>&1 > log
./bin/Debug/net7.0/linux-arm64/native/hello-4

Tested on x86-64 and arm64
@Beau-Gosse-dev
Copy link
Contributor

@smhmhmd and I both tested the recent commit across ARM64 and x64 with the below sample code, and were able to see that ulocdata_getCLDRVersion_66 was statically linked in the binary. As expected the resulting binary is much bigger with this new static linking. Up from 20MB unstripped to 50MB unstripped.

Published with ~/dotnet-sdk/dotnet publish /p:PublishAot=true /p:StaticICULinking=true -bl -r linux-arm64 --self-contained

namespace CurrentCulture
{
        class Program
        {
                static void Main(string[] args)
                {
                        double val = 1235.56;

                        Console.WriteLine($"Current culture: {CultureInfo.CurrentCulture.Name}");
                        Console.WriteLine(val);

                        CultureInfo.DefaultThreadCurrentCulture = new CultureInfo("en-US");

                        Console.WriteLine($"Current culture: {CultureInfo.CurrentCulture.Name}");
                        Console.WriteLine(val);
                }
        }
}

Ubuntu and others added 5 commits September 3, 2022 12:03
*  Incorporated change to use only src/native/libs/System.Globalization.Native/CMakeLists.txt
*  Conditionalized CMakeLists.txt for local build
*  Moved the build.sh into parent as local_build.sh and removed local-build directory.
*  Rename STATIC_SHIM_COMPILE to LOCAL_BUILD
Move the includes in Microsoft.DotNet.ILCompiler.pkgproj to
<ItemGroup Condition="'$(PackageTargetRuntime)' != ''">
Move the includes in Microsoft.DotNet.ILCompiler.pkgproj to
<ItemGroup Condition="'$(PackageTargetRuntime)' != ''">
@jkotas
Copy link
Member

jkotas commented Sep 4, 2022

One last thing: Could you please add a section "Using statically linked ICU" to https://github.com/dotnet/runtime/blob/main/src/coreclr/nativeaot/docs/compiling.md that documents this feature? (what it does and why one may want to use it, additional pre-requisites to install in addition to https://docs.microsoft.com/en-us/dotnet/core/deploying/native-aot/#prerequisites, etc.)

@smhmhmd
Copy link
Contributor Author

smhmhmd commented Sep 4, 2022

Could you please add a section "Using statically linked ICU" to https://github.com/dotnet/runtime/blob/main/src/coreclr/nativeaot/docs/compiling.md that documents this feature

Thanks, I will add in next patch

* Add "Using statically linked ICU" to compiling.md
* Remove define check for LOCAL_BUILD in CMakeLists.txt
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for working on this!

@dotnet/ilc-contrib @dotnet/area-system-globalization Any additional feedback?

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Looks great otherwise.

If you would like to make sure we're testing this configuration and it doesn't regress, add a test under src/tests/nativeaot/SmokeTests.

@smhmhmd
Copy link
Contributor Author

smhmhmd commented Sep 5, 2022

If you would like to make sure we're testing this configuration and it doesn't regress, add a test under src/tests/nativeaot/SmokeTests.

Thanks, I will add a test under src/tests/nativeaot/SmokeTests

@sbomer sbomer merged commit 7646e76 into dotnet:main Sep 6, 2022
@smhmhmd smhmhmd deleted the build branch September 6, 2022 23:38
@ghost ghost locked as resolved and limited conversation to collaborators Oct 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants