ILCompiler nuget package support by A-And · Pull Request #4983 · dotnet/corert

A-And · 2017-11-22T01:43:47Z

Below is a working implementation - CoreRT can be consumed by adding a

 <PackageReferenceInclude="Microsoft.DotNet.ILCompiler"Version="X" />

where X is the version of the ILCompiler package produced by build-packages.cmd or once its published with

dotnet add package Microsoft.DotNet.ILCompiler

Ideally, I'd like to add the assemblies produced by the project references in Microsoft.DotNet.ILCompiler.pkgproj to have their TargetPath property overriden when creating this package specifically, but the paths seem to be dynamically generated somewhere in the compiler. Can anyone more familiar with the NuGet package publishing pipeline take a look?
Additionally, the implementation below doesn't include symbols for the framework, sdk and tools assemblies to the final package.

jkotas · 2017-11-22T02:32:16Z

+ <!-- PackageTargetRuntime needs to be reset because if let unset, it's going to be set to globally configured value 
+ which will cause the package assets to be placed into runtime/[RID]/lib/[TFM]/ path, instead of lib/[TFM] -->
+
+ <ProjectReference Include="$(PackageSourceDirectory)ILCompiler\src\ILCompiler.csproj" >


Do we need these ProjectReferences at all?

Not specifically.
Ideally, I'd like to use the assemblies generated by these directly instead of including the entire 'tools' folder. I wanted to leave them here mostly so discussion is easier.

The package needs to contain the compiler as published app so that it can run (ie what you would get if you have done dotnet publish ILCompiler.csproj).
The "buildtools" msbuild targets that we are using currently do not support regular publishing easily, so we produce the bits in semi-complicated way. The result is about the same as what you would get by running dotnet publish ILCompiler.csproj ILCompiler.csproj -r <...>.
For the package reference to work well here, it would need to somehow do the publishing. And if the publishing works, it should be sufficient to have just the reference to ILCompiler.csproj since the publishing does transitive closure.

sergiy-k · 2017-11-22T07:51:11Z

+ "Microsoft.DotNet.ILCompiler" : {
+ "StableVersions": [],
+ "BaselineVersion": "1.0.0",
+ },


Nit: there is an extra space on this line.

jkotas · 2017-11-27T23:05:42Z

+ <!-- PackageTargetRuntime needs to be reset because if let unset, it's going to be set to globally configured value 
+ which will cause the package assets to be placed into runtime/[RID]/lib/[TFM]/ path, instead of lib/[TFM] -->

 <Dependency Include="Microsoft.DotNet.ObjectWriter">


I do not think ObjectWriter, NetCore.Jit and S.C.Immutable packages needs to be listed here at all.

(And if they need to be here for some reason - they should use versions from https://github.com/dotnet/corert/blob/master/dependencies.props)

A-And · 2017-11-30T21:58:38Z

+
+Once completed, you can find the native executable in the root folder of your project under ``/bin/x64/<Configuration>/netcoreapp2.0/publish/`` 
+
+## Try it out!


Wrong tone?

A-And · 2017-11-30T22:01:00Z

 <TargetOS Condition="'$(TargetOS)' == '' and '$(OS)' == 'Unix' and Exists('/Applications')">OSX</TargetOS>
 <TargetOS Condition="'$(TargetOS)' == ''">$(OS)</TargetOS>
+ <!-- Detect whether we're running the ILCompiler from a package and if so override the environmental variable to point to the correct assemblies -->
+ <IlcPath Condition="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), Microsoft.DotNet.ILCompiler.nuspec)) != ''">$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), Microsoft.DotNet.ILCompiler.nuspec))</IlcPath>


This feels a bit hacky. There could be a better approach.

Can this be just something like <IlcPath Condition="$(IlcPath) == ''">$(MSBuildThisFileDirectory)/..</IlcPath> ?

sergiy-k · 2017-12-01T01:59:42Z

+
+namespace SampleWebApi.Controllers
+{
+ public class ValuesController { 


Indentation is incorrect here.

sergiy-k · 2017-12-01T01:59:47Z

+ return new string[] { "value1", "value2" };
+ }
+
+ // This is the method that crashes, which Sergiy and I mentioned


Could you please open an issue to track this problem?

sergiy-k · 2017-12-01T02:05:06Z

+ </ItemGroup>
+
+ <ItemGroup>
+ <DotNetCliToolReference Include="Microsoft.VisualStudio.Web.CodeGeneration.Tools" Version="2.0.1" />


Do we really need this reference? I would remove it to keep the sample project simple.

sergiy-k · 2017-12-01T02:14:14Z

+ </PropertyGroup>
+
+ <ItemGroup>
+ <IlcArg Include="--noscan" />


Adding the "--noscan" compiler option should not be necessary after #5040 is merged. I would remove it from the sample.

sergiy-k · 2017-12-01T02:15:14Z

+
+``dotnet publish -c <Configuration> -r <RID>``
+
+where ``<Configuration>`` is your project configuratio (such as Debug or Release) and ``<RID>`` is the runtime identifier - one of ``win-x64``, ``win-x86``, ``linux-x64`` or ``osx-x64`` depending on the OS you want to publish for. For example, if you want to publish a release configuration of your app for a 64-bit version of Windows the command would look like:


Currently x86 is not supported by CoreRT. I would remove "win-x86"

sergiy-k · 2017-12-01T17:44:54Z

+
+where path_to_rdxml_file is the location of the file on your disk. Under the second ``<ItemGroup>`` remove the reference to ``Microsoft.AspNetCore.All`` and substitute it with:
+
+ <PackageReference Include="Microsoft.AspNetCore" Version="2.0.0-preview1-final" /> 


The version for ASP.NET packages should be Version="2.0.0". What you have is a pre-release versions.

sergiy-k · 2017-12-01T17:45:08Z

+ </ItemGroup>
+
+ <ItemGroup>
+ <PackageReference Include="Microsoft.AspNetCore" Version="2.0.0-preview1-final" /> 


The version for ASP.NET packages should be Version="2.0.0".

sergiy-k · 2017-12-01T17:46:20Z

+ <PackageReference Include="Microsoft.AspNetCore" Version="2.0.0-preview1-final" /> 
+ <PackageReference Include="Microsoft.AspNetCore.Mvc.Core" Version="2.0.0-preview1-final" /> 
+ <PackageReference Include="Microsoft.AspNetCore.Mvc.Formatters.Json" Version="2.0.0-preview1-final" /> 
+ <PackageReference Include="Microsoft.DotNet.ILCompiler" Version="1.0.0-alpha-25929-0" />


If it is possible, I would not hardcode a specific version of the compiler package. Ideally, the sample should run against the latest published one.

sergiy-k · 2017-12-01T17:49:28Z

@A-And, thank you!
If you don't mind, I would suggest to check in the sample as a separate PR. Basically:

Merge the compiler nuget package support
Make sure we have an official build published
Make the sample work against the latest officially published build and check the sample in.

A-And · 2017-12-01T18:20:12Z

@sergiy-k Sure thing! Would make certainly make more sense.

mikedn · 2017-12-01T21:15:05Z

FWIW I gave the package (1.0.0-alpha-25930-03, from yesterday) a try and it seems to be working fine, a native executable is produced. There are a number of linker warnings like:

bootstrapper.lib(main.obj) : warning LNK4099: PDB 'bootstrapper.pdb' was not found with 'bootstrapper.lib(main.obj)' or at 'D:\Projects\cda\bin\x64\Release\netcoreapp2.0\win-x64\native\bootstrapper.pdb'; linking object as if no debug info [D:\Projects\cda\cda.csproj]
Runtime.lib(MiscHelpers.obj) : warning LNK4099: PDB 'Runtime.pdb' was not found with 'Runtime.lib(MiscHelpers.obj)' or at 'D:\Projects\cda\bin\x64\Release\netcoreapp2.0\win-x64\native\Runtime.pdb'; linking object as if no debug info [D:\Projects\cda\cda.csproj]

Presumably the .pdb files for native static libraries like runtime.lib should be included in the package as well. The same way that VC++ installs both .lib and .pdb files for its static runtime libraries.

A-And · 2017-12-01T21:33:35Z

@mikedn Thanks for testing it out! The motivation behind excluding them was to keep the package as minimal as possible, but we should avoid linker warnings if possible.

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

@@ -23,6 +23,8 @@ See the LICENSE file in the project root for more information.
 <!-- Workaround for lack of current host OS detection - https://github.com/Microsoft/msbuild/issues/539 -->


BTW: This issue was fixed - can this be changed to use [MSBuild]::IsOSPlatform() now?

sergiy-k · 2017-12-01T23:49:40Z

@A-And, there should be no warning while compiling an app (i.e. the ones that @mikedn mentioned). We should either ship the pdb files or disable the warnings in the target files with a comment. But this should not block merging your PR. It can be done as a separate change. Thank you!

jkotas · 2017-12-02T00:49:49Z

- <TargetOS Condition="'$(TargetOS)' == '' and '$(OS)' == 'Unix' and Exists('/Applications')">OSX</TargetOS>
+ <TargetOS Condition="'$(TargetOS)' == '' and '[MSBuild]::IsOSPlatform('OSX')">OSX</TargetOS>
 <TargetOS Condition="'$(TargetOS)' == ''">$(OS)</TargetOS>
+ <!-- Detect whether we're running the ILCompiler from a package and if so override the environmental variable to point to the correct assemblies -->


Reposting https://github.com/dotnet/corert/pull/4983/files/746cfebb4cd9c19f9a7fd7e87c8f3e65c3ee196f..de7a9432439e46dbcbe65677dbf2267b6f44082c#r154461423 to make sure you have seen it:
Can this be just something like $(MSBuildThisFileDirectory)/.. , and remove the Error below?

A-And · 2017-12-02T01:09:52Z

@sergiy-k I've manually added the debug symbols for static libraries for now. All .pdbs are already published as part of the nuget symbol package, so we have to consider how a user debugging scenario should work.

jkotas · 2017-12-02T04:53:39Z

 <NativeOutputPath Condition="'$(NativeOutputPath)' == ''">$(OutputPath)native\</NativeOutputPath>
 <NativeCompilationDuringPublish Condition="'$(NativeCompilationDuringPublish)' == ''">true</NativeCompilationDuringPublish>
- <!-- Workaround for lack of current host OS detection - https://github.com/Microsoft/msbuild/issues/539 -->
- <TargetOS Condition="'$(TargetOS)' == '' and '$(OS)' == 'Unix' and Exists('/Applications')">OSX</TargetOS>


I am sorry for not being clear ... I mean to replace this line with the one that is using `[MSBuild]::IsOSPlatform. I suspect that this is why the CI tests on OSX are failing.

jkotas · 2017-12-02T04:53:54Z


 <MakeDir Directories="$([System.IO.Path]::GetDirectoryName($(NativeObject)))" />
-
+


Nit: extra whitespace

mikedn · 2017-12-02T07:08:09Z

All .pdbs are already published as part of the nuget symbol package

Hmm, it's unlikely that static library .pdbs need to be published, normally the debugger does not look for them, only the linker.

Those static .pdbs might be needed during debugging if the /DEBUG:FASTLINK linker option is used and ILC probably doesn't do that.

And even if it does, this is something that's supposed to work on the developer's machine, where static .pdbs already exist. In "release" scenarios, the .pdb produced by ILC should be a standalone .pdb. The debugger using it is not supposed to need static library .pdbs, .objs or any other intermediate build files.

jkotas · 2017-12-04T21:39:24Z

Sigh, the Windows build is failing with Method '[MSBuild]::IsOSPlatform' not found. Static method invocation should be of the form. The CI machines are not upgraded to latest VS/msbuild yet, and so we cannot remove the workaround.

Could you please keep the workaround in place? (Keep the new proper OS detection code around commented out.)

A-And · 2017-12-04T21:44:09Z

@jkotas Thanks for confirming, will do.

…orkaround

jkotas reviewed Nov 22, 2017
View reviewed changes

sergiy-k reviewed Nov 22, 2017
View reviewed changes

A-And force-pushed the ILCompilerPackageSupport branch from a95eb72 to df23f56Compare November 27, 2017 18:02

jkotas reviewed Nov 27, 2017
View reviewed changes

A-And force-pushed the ILCompilerPackageSupport branch from 2c9282e to e9959f6Compare November 27, 2017 23:06

A-And changed the title ~~[WIP] ILCompiler nuget package support~~ILCompiler nuget package supportNov 27, 2017

A-And changed the title ~~ILCompiler nuget package support~~[WIP] ILCompiler nuget package supportNov 27, 2017

A-And force-pushed the ILCompilerPackageSupport branch 3 times, most recently from 6248d9e to 9cee186Compare November 30, 2017 21:57

A-And commented Nov 30, 2017
View reviewed changes

A-And changed the title ~~[WIP] ILCompiler nuget package support~~ILCompiler nuget package supportNov 30, 2017

A-And force-pushed the ILCompilerPackageSupport branch from 75d4664 to 03ad3f7Compare November 30, 2017 22:17

sergiy-k reviewed Dec 1, 2017
View reviewed changes

A-And force-pushed the ILCompilerPackageSupport branch from 3b3c327 to aec5771Compare December 1, 2017 18:27

ILCompiler nuget package support
746cfeb

A-And force-pushed the ILCompilerPackageSupport branch from aec5771 to 746cfebCompare December 1, 2017 18:54

jkotas reviewed Dec 1, 2017
View reviewed changes

jkotas reviewed Dec 2, 2017
View reviewed changes

A-And force-pushed the ILCompilerPackageSupport branch 2 times, most recently from 5707e82 to 64cb37dCompare December 2, 2017 01:06

jkotas reviewed Dec 2, 2017
View reviewed changes

A-And force-pushed the ILCompilerPackageSupport branch 3 times, most recently from 3d2605b to 296f798Compare December 4, 2017 19:05

A-And force-pushed the ILCompilerPackageSupport branch from 296f798 to baff080Compare December 4, 2017 21:42

A-And force-pushed the ILCompilerPackageSupport branch 2 times, most recently from b33ad2c to c0f7aeaCompare December 4, 2017 21:56

Add symbols for native libraries to package and remove OS detection w…
3456f13
…orkaround

A-And force-pushed the ILCompilerPackageSupport branch from c0f7aea to 3456f13Compare December 4, 2017 22:40

Whitespace
cc9eeee

jkotas merged commit a35d222 into dotnet:masterDec 5, 2017

A-And mentioned this pull request Dec 19, 2017
[WIP] Package build #5133
Closed


		Once completed, you can find the native executable in the root folder of your project under ``/bin/x64/<Configuration>/netcoreapp2.0/publish/``

		## Try it out!


		``dotnet publish -c <Configuration> -r <RID>``

		where ``<Configuration>`` is your project configuratio (such as Debug or Release) and ``<RID>`` is the runtime identifier - one of ``win-x64``, ``win-x86``, ``linux-x64`` or ``osx-x64`` depending on the OS you want to publish for. For example, if you want to publish a release configuration of your app for a 64-bit version of Windows the command would look like:


		where path_to_rdxml_file is the location of the file on your disk. Under the second ``<ItemGroup>`` remove the reference to ``Microsoft.AspNetCore.All`` and substitute it with:

		<PackageReference Include="Microsoft.AspNetCore" Version="2.0.0-preview1-final" />

		@@ -23,6 +23,8 @@ See the LICENSE file in the project root for more information.
		<!-- Workaround for lack of current host OS detection - https://github.com/Microsoft/msbuild/issues/539 -->


		<MakeDir Directories="$([System.IO.Path]::GetDirectoryName($(NativeObject)))" />

Uh oh!

Conversation

A-And commented Nov 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotasNov 22, 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!

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!

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!

sergiy-k commented Dec 1, 2017

Uh oh!

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

Uh oh!

Uh oh!

mikedn commented Dec 1, 2017

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergiy-k commented Dec 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

A-And commented Dec 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikedn commented Dec 2, 2017

Uh oh!

jkotas commented Dec 4, 2017

Uh oh!

A-And commented Dec 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jkotasNov 22, 2017•
edited
Loading

A-And commented Dec 1, 2017•
edited
Loading

A-And commented Dec 1, 2017•
edited
Loading