Adding basic support for an ilproj sdk. #317

tannergooding · 2018-07-12T16:10:01Z

This adds the basic support for an ilproj sdk. The IL compilation target was taken from the CoreFX IL.targets

tannergooding · 2018-07-12T16:10:10Z

tannergooding · 2018-07-12T16:11:18Z

A bare bones ilproj would look like:

<?xml version="1.0"?>
<Project Sdk="Microsoft.NET.Sdk.IL">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp2.1</TargetFramework>
  </PropertyGroup>

</Project>

I validated locally that this works with System.Runtime.CompilerServices.Unsafe.ilproj

tannergooding · 2018-07-12T16:12:21Z

The SDK doesn't have all the support required to open an ilproj in VS, but that can always be added in the future (if desired).

src/Microsoft.DotNet.Sdk.IL/Microsoft.DotNet.Sdk.IL.nuspec

+<?xml version="1.0" encoding="utf-8"?>
+<package>
+  <metadata minClientVersion="2.8.1">
+    <id>Microsoft.DotNet.Sdk.IL</id>


tannergooding · 2018-07-12T16:16:06Z

Also CC. @eerhardt

src/Microsoft.DotNet.Sdk.IL/build/Microsoft.NET.Sdk.IL.targets

+  </PropertyGroup>
+
+  <PropertyGroup>
+    <OSPlatform Condition="$([System.Runtime.InteropServices.RuntimeInformation]::IsOSPlatform($([System.Runtime.InteropServices.OSPlatform]::Windows)))">win</OSPlatform>


src/Microsoft.DotNet.Sdk.IL/build/Microsoft.NET.Sdk.IL.targets

+  </PropertyGroup>
+
+  <ItemGroup>
+    <PackageReference Include="$(IlasmPackageName)" Version="$(IlasmPackageVersion)" />


src/Microsoft.DotNet.Sdk.IL/build/Microsoft.NET.Sdk.IL.targets

+
+      <_KeyFileArgument Condition="'$(KeyOriginatorFile)' != ''">-KEY=$(KeyOriginatorFile)</_KeyFileArgument>
+
+      <_IlasmSwitches>-QUIET -NOLOGO</_IlasmSwitches> 


src/Microsoft.DotNet.Sdk.IL/build/Microsoft.NET.Sdk.IL.targets

+  </PropertyGroup>
+
+  <PropertyGroup>
+    <OSPlatform Condition="$([System.Runtime.InteropServices.RuntimeInformation]::IsOSPlatform($([System.Runtime.InteropServices.OSPlatform]::Windows)))">win</OSPlatform>


src/Microsoft.DotNet.Sdk.IL/build/Microsoft.NET.Sdk.IL.targets

+  <Target Name="CreateManifestResourceNames"
+          Condition="'@(EmbeddedResource)' != ''"
+          DependsOnTargets="$(CreateManifestResourceNamesDependsOn)">
+    <!-- Required by Microsoft.Common.targets -->


src/Microsoft.DotNet.Sdk.IL/build/Microsoft.NET.Sdk.IL.targets

+    </Exec>
+    <Error Text="ILAsm failed" Condition="'$(_ILAsmExitCode)' != '0'" />
+
+    <CallTarget Targets="$(TargetsTriggeredByCompilation)" Condition="'$(TargetsTriggeredByCompilation)' != ''"/>


src/Microsoft.DotNet.Sdk.IL/sdk/Sdk.targets

+
+  <PropertyGroup>
+    <MSBuildAllProjects>$(MSBuildThisFileFullPath);$(MSBuildAllProjects)</MSBuildAllProjects>
+    <LanguageTargets Condition="'$(MSBuildProjectExtension)' == '.ilproj'">$(MSBuildThisFileDirectory)..\build\Microsoft.NET.Sdk.IL.targets</LanguageTargets>


src/Microsoft.DotNet.Sdk.IL/build/Microsoft.NET.Sdk.IL.targets

+
+WARNING:  DO NOT MODIFY this file unless you are knowledgeable about MSBuild and have
+          created a backup copy.  Incorrect changes to this file will make it
+          impossible to load or build your projects from the command-line or the IDE.


weshaggard · 2018-07-12T16:59:09Z

@RussKeldorph PTAL we are doing some work to create a package for ilprojs.

src/Microsoft.DotNet.Sdk.IL/build/Microsoft.NET.Sdk.IL.targets

+    <RuntimeIdentifier>$(OSPlatform)-$(OSArchitecture.ToLower())</RuntimeIdentifier>
+    <CoreCLRPackageName>runtime.$(RuntimeIdentifier).microsoft.netcore.runtime.coreclr</CoreCLRPackageName>
+    <IlasmPackageName>runtime.$(RuntimeIdentifier).microsoft.netcore.ilasm</IlasmPackageName>
+    <IlasmPackageVersion>2.0.8</IlasmPackageVersion>


src/Microsoft.DotNet.Sdk.IL/build/Microsoft.NET.Sdk.IL.targets

+
+  <ItemGroup>
+    <PackageReference Include="$(IlasmPackageName)" Version="$(IlasmPackageVersion)" />
+    <PackageReference Include="$(CoreCLRPackageName)" Version="$(IlasmPackageVersion)" />


src/Microsoft.DotNet.Sdk.IL/build/Microsoft.NET.Sdk.IL.targets

+
+    <Error Condition="'$(ToolsDir)' == ''" Text="ToolsDir must be set in order to build ilproj's" />
+    <MakeDir Directories="$(ToolsDir)\ilasm" />
+    <Copy DestinationFolder="$(ToolsDir)\ilasm" SourceFiles="@(_IlasmSourceFiles)" />


src/Microsoft.DotNet.Sdk.IL/sdk/Sdk.props

+  </PropertyGroup>
+
+  <Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk" />
+  <Import Project="$(MSBuildThisFileDirectory)..\build\Microsoft.NET.Sdk.IL.props" Condition="'$(MSBuildProjectExtension)' == '.ilproj'" />


RussKeldorph · 2018-07-12T19:58:49Z

@chsienki @AaronRobinsonMSFT FYI

src/Microsoft.DotNet.Sdk.IL/Microsoft.DotNet.Sdk.IL.csproj

+<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <TargetFrameworks>netstandard1.0</TargetFrameworks>


src/Microsoft.DotNet.Sdk.IL/build/Microsoft.NET.Sdk.IL.targets

+    <RuntimeIdentifier>$(OSPlatform)-$(OSArchitecture.ToLower())</RuntimeIdentifier>
+    <CoreCLRPackageName>runtime.$(RuntimeIdentifier).microsoft.netcore.runtime.coreclr</CoreCLRPackageName>
+    <IlasmPackageName>runtime.$(RuntimeIdentifier).microsoft.netcore.ilasm</IlasmPackageName>
+    <IlasmPackageVersion>2.0.8</IlasmPackageVersion>


tannergooding · 2018-07-12T20:36:02Z

Resolved the majority of feedback so far (that isn't part of secondary refactoring/cleanup work)

src/Microsoft.DotNet.Sdk.IL/build/Microsoft.NET.Sdk.IL.targets

+    <OSPlatform Condition="$([MSBuild]::IsOSPlatform('linux'))">linux</OSPlatform>
+    <OSPlatform Condition="$([MSBuild]::IsOSPlatform('osx'))">osx</OSPlatform>
+    <OSArchitecture>$([System.Runtime.InteropServices.RuntimeInformation]::OSArchitecture)</OSArchitecture>
+    <RuntimeIdentifier Condition="'$(RuntimeIdentifier)' == ''">$(OSPlatform)-$(OSArchitecture.ToLower())</RuntimeIdentifier>


src/Microsoft.DotNet.Sdk.IL/build/Microsoft.NET.Sdk.IL.props

@@ -0,0 +1,19 @@
+<?xml version="1.0" encoding="utf-8"?>


src/Microsoft.DotNet.Sdk.IL/build/Microsoft.NET.Sdk.IL.targets

+  </PropertyGroup>
+
+  <PropertyGroup>
+    <OSPlatform Condition="$([MSBuild]::IsOSPlatform('windows'))">win</OSPlatform>


src/Microsoft.DotNet.Sdk.IL/build/Microsoft.NET.Sdk.IL.targets

+    <IlasmPackageRuntimeId Condition="'$(IlasmPackageRuntimeId)' == ''">$(OSPlatform)-$(OSArchitecture.ToLower())</IlasmPackageRuntimeId>
+    <CoreCLRPackageName>runtime.$(IlasmPackageRuntimeId).microsoft.netcore.runtime.coreclr</CoreCLRPackageName>
+    <IlasmPackageName>runtime.$(IlasmPackageRuntimeId).microsoft.netcore.ilasm</IlasmPackageName>
+    <IlasmPackageVersion Condition="'$(IlasmPackageVersion)' == ''">2.0.8</IlasmPackageVersion>


src/Microsoft.DotNet.Sdk.IL/build/Microsoft.NET.Sdk.IL.targets

+      <_IlasmSourceFiles Include="$(NuGetPackageRoot)\$(CoreCLRPackageName)\$(IlasmPackageVersion)\runtimes\$(IlasmPackageRuntimeId)\native\**\*" />
+    </ItemGroup>
+
+    <Error Condition="'$(ToolsDir)' == ''" Text="ToolsDir must be set in order to build ilproj's" />


src/Microsoft.DotNet.Sdk.IL/Microsoft.DotNet.Sdk.IL.csproj

+
+  <ItemGroup>
+    <None Include="sdk/*.*" Pack="true" PackagePath="sdk/%(Filename)%(Extension)" />
+    <None Include="build/*.*" Pack="true" PackagePath="build/%(Filename)%(Extension)" />


eerhardt · 2018-07-12T23:55:09Z

Have we given any thought to how source-build and bootstrapping a new platform will work with these changes?

Check out: https://github.com/dotnet/source-build/blob/master/Documentation/boostrap-new-os.md#use-the-bootstrap-cli-to-build-source-build for some hack workarounds I needed to do with ILAsm when trying to bootstrap a platform.

One concern here is the ordering of the repos during source-build. The way we have it today is that roslyn-tools (i.e. the current 'arcade') is built first, but here are we directly dependent on coreclr. I'm wondering how someone could fix a bug in ILAsm, and then use source-build to build the product again. How could someone tell this new IL SDK package to use the local ILAsm package that they just built themself?

NOTE: this initial PR doesn't need to block on this comment, just putting up some "food for thought".

tannergooding · 2018-07-13T16:35:24Z

How could someone tell this new IL SDK package to use the local ILAsm package that they just built themself?

I would presume that CoreCLR builds the package and puts it into the source build feed. Source build when then consume said package from the source build feed (it should be no different from dogfooding any other native tool from the toolchain).

eerhardt · 2018-07-13T16:53:33Z

I would presume that CoreCLR builds the package and puts it into the source build feed. Source build when then consume said package from the source build feed

But the way you have it coded, your package uses the 2.0.8 version. Who will tell it to use the one that is in the source build feed?

tannergooding · 2018-07-13T17:01:13Z

But the way you have it coded, your package uses the 2.0.8 version.

It's conditionalized so anyone can override it. That being said, it isn't following the standard naming convention, which I'll update.

tannergooding · 2018-07-13T17:06:19Z

Updated as per the new feedback so far.

weshaggard · 2018-07-13T17:40:36Z

I would presume that CoreCLR builds the package and puts it into the source build feed. Source build when then consume said package from the source build feed (it should be no different from dogfooding any other native tool from the toolchain).

Is there any reason we don't just put it in coreclr to start with?

tannergooding · 2018-07-13T17:43:03Z

Is there any reason we don't just put it in coreclr to start with

ILAsm and the ILAsm package are both in CoreCLR and produced from there already... Unless you are asking about this SDK package?

weshaggard · 2018-07-13T17:44:24Z

Unless you are asking about this SDK package?

Yes I'm referring to the sdk package. Ideally we can keep everything together. We could even potentially turn the root Microsoft.NETCore.Ilasm package into the sdk package.

tannergooding · 2018-07-13T17:59:35Z

We could even potentially turn the root Microsoft.NETCore.Ilasm package into the sdk package.

I'm not sure if that is desirable.

ilasm is a good choice for a standalone tool, and it probably shouldn't be only shipped within the ilproj SDK. If we moved it into CoreCLR, I think we would want to have both the ilasm standalone package and the ilproj SDK.... I think it might be a good idea to also ship the ilasm tool direclty in the ilproj SDK (rather than downloading it via a pkgref), however.

eerhardt · 2018-07-13T18:03:18Z

Compare it to our other compiler NuGet package - https://www.nuget.org/packages/Microsoft.NETCore.Compilers/ has csc and vbc in it.

tannergooding · 2018-07-13T18:07:14Z

Compare it to our other compiler NuGet package - https://www.nuget.org/packages/Microsoft.NETCore.Compilers/ has csc and vbc in it.

Right. The Microsoft.NETCore.Compilers package contains just the tool and supporting targets. It hooks into the existing SDK (via hooks we added to the SDK for C#/VB), when referenced; rather than being a new SDK itself.

weshaggard · 2018-07-13T23:18:33Z

@tannergooding at any rate to help with source-build and other such things I think it would be good to have this SDK package build in coreclr instead of arcade so the tools can be built as a prereq for the other higher level repo's needing this.

tannergooding · 2018-07-16T16:31:30Z

Working on porting this over to CoreCLR. Will close after the new PR is up.

tannergooding · 2018-07-16T22:09:39Z

Porting to CoreCLR requires a change to build-tools so that the pkgproj in CoreCLR can be marked with PackgeType=MSBuildSdk

weshaggard · 2018-07-16T23:12:42Z

Porting to CoreCLR requires a change to build-tools so that the pkgproj in CoreCLR can be marked with PackgeType=MSBuildSdk

That's fine go ahead and make the necessary PR changes.

tannergooding · 2018-07-19T16:48:18Z

As an update. The BuildTools changes went in: dotnet/buildtools#2100

Just waiting on the changes to be pumped back to CoreCLR now.

tannergooding · 2018-07-20T21:14:28Z

Replaced by dotnet/coreclr#19066

tannergooding · 2018-07-31T15:37:20Z

CoreFX side is here: dotnet/corefx#31512

tmat reviewed Jul 12, 2018

View reviewed changes

tannergooding commented Jul 12, 2018

View reviewed changes

tmat reviewed Jul 12, 2018

View reviewed changes

rainersigwald reviewed Jul 12, 2018

View reviewed changes

weshaggard reviewed Jul 12, 2018

View reviewed changes

jaredpar reviewed Jul 12, 2018

View reviewed changes

weshaggard reviewed Jul 12, 2018

View reviewed changes

eerhardt reviewed Jul 12, 2018

View reviewed changes

src/Microsoft.DotNet.Sdk.IL/build/Microsoft.NET.Sdk.IL.targets Outdated

</PropertyGroup>

<PropertyGroup>

<OSPlatform Condition="$([MSBuild]::IsOSPlatform('windows'))">win</OSPlatform>

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

eerhardt reviewed Jul 12, 2018

View reviewed changes

Adding basic support for an ilproj sdk.

4718189

tannergooding mentioned this pull request Jul 17, 2018

Updating Packaging.targets to support specifying the PackageType dotnet/buildtools#2100

Merged

tannergooding mentioned this pull request Jul 20, 2018

Adding basic support for an ilproj sdk dotnet/coreclr#19066

Merged

tannergooding closed this Jul 20, 2018


		<_KeyFileArgument Condition="'$(KeyOriginatorFile)' != ''">-KEY=$(KeyOriginatorFile)</_KeyFileArgument>

		<_IlasmSwitches>-QUIET -NOLOGO</_IlasmSwitches>

Adding basic support for an ilproj sdk. #317

Adding basic support for an ilproj sdk. #317

Uh oh!

Conversation

tannergooding commented Jul 12, 2018

Uh oh!

tannergooding commented Jul 12, 2018

Uh oh!

tannergooding commented Jul 12, 2018

Uh oh!

tannergooding commented Jul 12, 2018

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

tannergooding commented Jul 12, 2018

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

weshaggard commented Jul 12, 2018

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!