ILCompiler Package Multi-Runtime Package Support #5123

A-And · 2017-12-15T18:56:39Z

Dupe of #5106 - I couldn't reopen the PR, because of overenthusiastic commit squashing.

As it stands we only produce a Windows ILCompiler package. This work implements building and publishing of OS-specific runtime packages and spins off ILCompiler as a meta-package, which can be referenced when adding to a project.

Working on this, a quirk in MSBuild behavior under .NET Core popped up - build artifacts (i.e. .targets and .props files) are imported for direct project package references, but not for runtime-specific packages, defined as dependencies in the meta-package. This doesn't seem to be the case in vanilla MSBuild.

The below is a serious hack to work around this - during runtime, we find the resolved runtime package reference and define the path to it on disk, from which all OS-specific components are loaded and run. The motivation behind the workaround was to keep the package as small as possible, particularly because of the large intersection of components between OS implementations.

All workarounds are marked as such and should be removed once dotnet/msbuild#2807 is resolved.

There is some slowdown because of sequential resolution of targets, but not immediately noticeable and is unfortunately unavoidable - almost entirely avoided if CoreRT build targets are referenced directly (excluding MSBuild overhead).

jkotas · 2017-12-15T19:02:25Z

There is extra binary file: PInvokeNative.exp

jkotas · 2017-12-15T19:27:59Z

pkg/Microsoft.DotNet.ILCompiler/Microsoft.DotNet.ILCompiler.builds

 <Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <TargetOS Condition="'$(TargetOS)' == '' and '$(OS)' == 'Unix' and Exists('/Applications')">OSX</TargetOS>


This should rather use OSGroup property. We want the build to target the OS that it was told on the command, not the OS that it is running on.

jkotas · 2017-12-15T19:35:51Z

pkg/Microsoft.DotNet.ILCompiler/Microsoft.DotNet.ILCompiler.pkgproj

+
  <ItemGroup>
-    <File Include="$(MSBuildThisFileDirectory)\Microsoft.DotNet.ILCompiler.targets" >
+    <ProjectReference Include="win\Microsoft.DotNet.ILCompiler.pkgproj" />


Do we want to set this up such that it works for win-x64, win-x86, linux-arm, ... ?

I thought these runtime specific packages can be structured such that the metapackage does not need to know about every runtime specific package associated with it. E.g. https://dotnet.myget.org/feed/dotnet-core/package/nuget/Microsoft.NETCore.App does not know that https://dotnet.myget.org/feed/dotnet-core/package/nuget/runtime.alpine.3.6-x64.Microsoft.NETCore.App exists.

That actually seems correct.
I made the wrong assumption that the metapackage's runtime.json was generated from its .pkgproj, but this isn't the case. As long as the runtime-specific packages have a dedicated package definition in a sub-folder, this is done automatically.
The current setup, however does only build packages according to which machine we're running on. Is cross-building of packages something we're looking into supporting?

Eventually - yes.

I expect that the eventual CoreRT platform matrix would be same as CoreCLR. So the configs where we cross-build for CoreCLR like linux-arm would cross build for CoreRT as well.

We do not need to be doing a bunch of extra work for cross-building right now. But we should be making things cross-building friendly where we can.

…finition

jkotas · 2017-12-15T20:24:14Z

pkg/Microsoft.DotNet.ILCompiler/Microsoft.DotNet.ILCompiler.builds

    </Project>
+
+    <!-- Include platform-specific package definitions-->
+    <Project Include="$(TargetRuntimePackageLocation)\Microsoft.DotNet.ILCompiler.pkgproj" />


Are these individual .pkgproj in subdirectories still required now that you do not include them from the metapackage?

I do not see them where we do similar things in other repos. E.g. https://github.com/dotnet/corefx/tree/master/pkg/Microsoft.Private.CoreFx.NETCoreApp .

Yes. Removing them would cause the runtime packages to not be built. CoreFX's packaging pipeline is much more involved and a lot of the package building for specific platforms is handled outside of the pkproj (most of the logic seems to be here: https://github.com/dotnet/corefx/blob/master/pkg/frameworkPackage.targets). In CoreRT I think this adds some unnecessary complexity, particularly because the package consists entirely of components built by the standard build process and everything else is handled by msbuild.

I based the subdirectory structure on guidelines here
https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/package-projects.md#platform-specific-library

I based the subdirectory structure on guidelines here
https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/package-projects.md#platform-specific-library

This document is outdated. We are not using this style of OS-specific packaging anywhere anymore.

If you replace TargetRuntimePackageLocation with say "TargetSpecific" and have just one unified .pkgproj shared by all OSes/architectures in "TargetSpecific" subdirectory, is it going to work? Or do the build scripts have a strong dependency on the win/linux/osx directory names.

I would like to minimize the amount of plumbing that needs to be done per new OS target.

Seems like the requirements are on a separate .pkgproj file with the same name as the lineup package, which has its RID explicitly set.
Now adding a new OS target should just involve setting a PackageTargetRuntime property during the build process - no need to explicitly add anything.

jkotas · 2017-12-15T22:28:45Z

pkg/Microsoft.DotNet.ILCompiler/Microsoft.DotNet.ILCompiler.targets

-<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+<Project 
+  xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>


It would be nice to move this file into the BuildIntegrationDirectory. The pkg/... should have just the files used to build the packages, not the files that go into the packages.

jkotas · 2017-12-15T22:31:39Z

src/BuildIntegration/Microsoft.NETCore.Native.targets

    <IlcCompileDependsOn Condition="'$(BuildingFrameworkLibrary)' != 'true'">Compile;ComputeIlcCompileInputs</IlcCompileDependsOn>
    <IlcCompileDependsOn Condition="'$(IlcMultiModule)' == 'true' and '$(BuildingFrameworkLibrary)' != 'true'">$(IlcCompileDependsOn);BuildFrameworkLib</IlcCompileDependsOn>
+    <IlcCompileDependsOn Condition="'$(TargetOS)' == 'Windows_NT'">$(IlcCompileDependsOn);SetupWindowsProps</IlcCompileDependsOn>  
+    <IlcCompileDependsOn Condition="'$(TargetOS)' != 'Windows_NT'">$(IlcCompileDependsOn);SetupUnixProps</IlcCompileDependsOn>  


Would it make sense for SetupWindowsProps/SetupUnixProps to have same name on both Windows and Unix?

I'm not sure. Ultimately only one set of .props will be loaded at any given build, so it would make sense to abstract that detail away from Native.targets and avoid another OS check. I chose specifically-named build targets to make build logs explicit - property setup would be easily identifiable.
That being said, I think this suggestion is the better approach.

It is not unusual to have the same target defined in multiple files. For example, CoreCompile is a well known target that works like that.

jkotas · 2017-12-15T22:35:38Z

src/BuildIntegration/Microsoft.NETCore.Native.targets


    <SharedLibrary Condition="'$(OS)' == 'Windows_NT'">$(FrameworkLibPath)\Framework$(LibFileExt)</SharedLibrary>
    <SharedLibrary Condition="'$(OS)' != 'Windows_NT'">$(FrameworkLibPath)\libframework$(LibFileExt)</SharedLibrary>
+    <DynamicBuildPropertyDependencies Condition="'$(CalledViaPackage)' == 'true'">SetupProperties</DynamicBuildPropertyDependencies>


It may be useful to have Ilc prefix e.g. IlcDynamicBuildPropertyDependencies for all these generically looking properties. It would make it obvious that the property is specific to Ilc, not a msbuild built-in.

jkotas · 2017-12-15T23:35:41Z

pkg/Microsoft.DotNet.ILCompiler/Microsoft.DotNet.ILCompiler.pkgproj

    </File>
-    <File Include="$(PackageSourceDirectory)\BuildIntegration\*">
+
+    <File Include="$(PackageSourceDirectory)\BuildIntegration\*" Exclude="$(PackageSourceDirectory)\BuildIntegration\Microsoft.DotNet.ILCompiler.targets">


Is there a reason why we have the one target in the build subdirectory and other in the targets subdirectory? Can we just have all of them in the build subdirectory?

From what I can tell, other nuget packages seems to be authored that way - they just have the targets in the build directory.

(If there is a good reason, I would be nice to add comment about it.)

Not really - I wanted to make a distinction between targets that are imported into the project at restore-time versus others, which are consumed at build-time.
Moved.

jkotas

LGTM otherwise

jkotas · 2017-12-16T00:11:11Z

pkg/Microsoft.DotNet.ILCompiler/Microsoft.DotNet.ILCompiler.builds

 <Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <TargetRuntimePackageLocation Condition="'$(OSGroup)' == 'Windows_NT'">win</TargetRuntimePackageLocation>


TargetRuntimePackageLocation can be deleted now. No longer used.

jkotas · 2017-12-16T00:12:39Z

pkg/Microsoft.DotNet.ILCompiler/TargetSpecific/Microsoft.DotNet.ILCompiler.pkgproj

@@ -0,0 +1,14 @@
+<!-- The ILCompiler package is produced by including built binaries and executables from the OS-Specific builds.
+  These contents are defined in Microsoft.Dotnet.ILCompiler.Common.props - the purpose of this file is to force the build process to produce runtime-specific packages. -->


Microsoft.Dotnet.ILCompiler.Common.props does not need to be separate file now. It can be just folded into this one.

jkotas · 2017-12-16T00:34:51Z

src/BuildIntegration/Microsoft.DotNet.ILCompiler.targets

+    <TargetOS Condition="'$(TargetOS)' == '' and '$(OS)' == 'Unix' and Exists('/Applications')">OSX</TargetOS>
+    <!-- The correct OS detection code. Uncomment once CI machines are upgraded to the latest version of MSBuild -->
+    <!-- <TargetOS Condition="'$([MSBuild]::IsOSPlatform(OSX))' == 'true'">OSX</TargetOS>  -->
+    <TargetOS Condition="'$(TargetOS)' == ''">$(OS)</TargetOS>


This is ok for now.

Eventually, we should just use the target that is passed on the dotnet publish command line via -r switch.

jkotas · 2017-12-16T01:10:04Z

src/BuildIntegration/BuildFrameworkNativeObjects.proj


-  <Import Project="Microsoft.NETCore.Native.targets" />
+  <!-- Part of workaround for lack of secondary build artifact import - https://github.com/Microsoft/msbuild/issues/2807 -->
+  <Import Project="$(MSBuildThisFileDirectory)..\build\Microsoft.DotNet.ILCompiler.targets" Condition="'$(IlcCalledViaPackage)' == 'true'"/>


..\build should not be needed here.

jkotas · 2017-12-16T01:10:56Z

src/BuildIntegration/Microsoft.NETCore.Native.targets

  </Target>

-  <Import Project="$(MSBuildThisFileDirectory)\Microsoft.NETCore.Native.Publish.targets" Condition="'$(NativeCompilationDuringPublish)' != 'false'" />
+  <Import Project="$(IlcBuildIntegrationTargetsDir)Microsoft.NETCore.Native.Publish.targets" Condition="'$(NativeCompilationDuringPublish)' != 'false'" />


Do we still need IlcBuildIntegrationTargetsDir ? It seems like that it can be just MSBuildThisFileDirectory everewhere.

(Or not needed in some places - because of the targets are in the same dir now.)

ILCompiler Package Multi-Runtime Package Support

efe6eb4

A-And force-pushed the ILCompilerPackageMultiRuntime branch from 74b5e34 to efe6eb4 Compare December 15, 2017 19:01

Remove extra binary

48e309f

jkotas reviewed Dec 15, 2017

View reviewed changes

Change OS-Detection and remove ProjectReferences from meta-package de…

6fb901f

…finition

jkotas reviewed Dec 15, 2017

View reviewed changes

Adding Formatting and Naming changes for clarity

4113bc6

jkotas reviewed Dec 15, 2017

View reviewed changes

A-And force-pushed the ILCompilerPackageMultiRuntime branch from e242a8f to 5dbd605 Compare December 16, 2017 00:00

Unify runtime-specific packages and move package build targets

f9802d8

A-And force-pushed the ILCompilerPackageMultiRuntime branch from 5dbd605 to f9802d8 Compare December 16, 2017 00:17

jkotas approved these changes Dec 16, 2017

View reviewed changes

Use command-line arg RID and remove unnecessary files

17d1c0c

A-And force-pushed the ILCompilerPackageMultiRuntime branch from 4bfe20b to 17d1c0c Compare December 16, 2017 00:53

jkotas reviewed Dec 16, 2017

View reviewed changes

A-And added 2 commits December 15, 2017 17:17

Respond to PR feedback

7c36fb0

Rename + add comments

c65eb32

jkotas merged commit df56bb3 into dotnet:master Dec 16, 2017

A-And deleted the ILCompilerPackageMultiRuntime branch December 16, 2017 02:52

A-And mentioned this pull request Dec 19, 2017

[WIP] Package build #5133

Closed

MichalStrehovsky mentioned this pull request Jul 19, 2022

Native AOT runtime changes for multiple package conflict dotnet/runtime#72346

Merged

LakshanF mentioned this pull request Jul 19, 2022

[NativeAOT] Clean up AOT Publishing dotnet/runtime#72415

Closed

		@@ -0,0 +1,14 @@
		<!-- The ILCompiler package is produced by including built binaries and executables from the OS-Specific builds.
		These contents are defined in Microsoft.Dotnet.ILCompiler.Common.props - the purpose of this file is to force the build process to produce runtime-specific packages. -->

ILCompiler Package Multi-Runtime Package Support #5123

ILCompiler Package Multi-Runtime Package Support #5123

Uh oh!

Conversation

A-And commented Dec 15, 2017

Uh oh!

jkotas commented Dec 15, 2017

Uh oh!

jkotas Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

A-And Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

A-And Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

A-And Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

A-And Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

jkotas Dec 15, 2017 •

edited

Loading

jkotas Dec 15, 2017 •

edited

Loading

A-And Dec 15, 2017 •

edited

Loading

jkotas Dec 15, 2017 •

edited

Loading

A-And Dec 15, 2017 •

edited

Loading

jkotas Dec 15, 2017 •

edited

Loading

A-And Dec 15, 2017 •

edited

Loading

A-And Dec 15, 2017 •

edited

Loading

jkotas Dec 16, 2017 •

edited

Loading