Updating S.R.CS.Unsafe to be built using the ilproj sdk. #31512

tannergooding · 2018-07-31T15:35:17Z

This moves off the $(ToolsDir)\IL.targets logic and onto the ilproj-sdk.

tannergooding · 2018-07-31T15:35:32Z

tannergooding · 2018-07-31T15:35:57Z

global.json

@@ -0,0 +1,5 @@
+{
+  "msbuild-sdks": {
+    "Microsoft.NET.Sdk.IL": "3.0.0-preview1-26731-07"


This should be hooked up to maestro. Not sure how to do that, however.

Check out https://github.com/dotnet/corefx/blob/master/dependencies.props#L181-L185.

tools-local\ILAsmVersion.txt should be removed. since it is no longer necessary.

That UpdateStep should be changed to instead point to this file, and it should update based on new CoreCLR builds (not BuildTools).

You can use a Regex updater for now, but we should probably write an msbuild-sdks updater at some point. /cc @dagood

In reply to: 206577961 [](ancestors = 206577961)

Along with deleting tools-local\ILAsmVersion.txt, we will need to update init-tools to no longer use it.

In reply to: 206583480 [](ancestors = 206583480,206577961)

We should also update the steps here:

https://github.com/dotnet/source-build/blob/master/Documentation/boostrap-new-os.md#use-the-bootstrap-cli-to-build-source-build

TODO: make this next step better - possibly by adding a new build parameter that takes the path to ILASM. Now, we can initialize our build tools with the bootstrap CLI and the ILASM tool.

In reply to: 206584850 [](ancestors = 206584850,206583480,206577961)

You can use a Regex updater for now

I don't think there's currently a way to set up a "pure" regex updater as MSBuild config, unfortunately... we'd need a new case in BaseDependenciesTask.cs#L95 for that. Or just go all the way to a JSON or msbuild-sdks updater.

Can this be done before merging, or does it need to be done after?

Nevermind, it looks like it can all be done as part of this PR.

eerhardt · 2018-07-31T15:50:31Z

This is not necessary.

Refers to: src/System.Runtime.CompilerServices.Unsafe/src/System.Runtime.CompilerServices.Unsafe.ilproj:1 in 6fb0efe. [](commit_id = 6fb0efe, deletion_comment = False)

eerhardt · 2018-07-31T15:51:18Z

src/System.Runtime.CompilerServices.Unsafe/Directory.Build.props

    <AssemblyVersion>4.0.5.0</AssemblyVersion>
    <AssemblyKey>MSFT</AssemblyKey>
    <IsUAP>true</IsUAP>
-    <LanguageTargets Condition="'$(MSBuildProjectExtension)' == '.ilproj'">$(ToolsDir)\IL.targets</LanguageTargets>


Are we going to delete the IL.targets from buildtools? It would be good to rip it out so we know that it is no longer being used.

First, CoreCLR needs to move all of its ilproj to the SDK as well.

CC. @AaronRobinsonMSFT

tannergooding · 2018-08-09T17:39:15Z

This should be done and working, end-to-end, now. CC. @eerhardt and @dagood for another review pass.

tannergooding · 2018-08-09T17:39:54Z

Directory.Build.props

    <PackagesDir>$(DotNetRestorePackagesPath)</PackagesDir>
    <PackagesDir Condition="'$(PackagesDir)'==''">$(ProjectDir)packages/</PackagesDir>
+    <RestorePackagesPath Condition="'$(RestorePackagesPath)'==''">$(PackagesDir)</RestorePackagesPath>
+    <NuGetPackageRoot Condition="'$(NuGetPackageRoot)'==''">$(PackagesDir)</NuGetPackageRoot>


Due to the way restore happens for CoreFX, we need to ensure that this is set so that it can be used by the ilproj-sdk.

tannergooding · 2018-08-09T17:40:37Z

external/runtime/runtime.depproj

      <!-- Following is version used in release/uwp6.0 -->
      <Version Condition="'$(TargetGroup)'=='uap10.0.16299'">2.1.0-b-uwp6-25707-02</Version>
    </PackageReference>
+    <PackageReference Include="Microsoft.NETCore.ILAsm">


This also needs to be set here as a side-effect of how CoreFX does restore. If you restore/build the ilproj directly, this isn't needed.

tannergooding · 2018-08-09T17:41:18Z

src/Directory.Build.targets

    <!-- Workaround https://github.com/dotnet/project-system/issues/3670 by setting TargetFramework expicitly -->
    <!-- This property is set late in the project evaluation so other properties don't try to use it. -->
-    <TargetFramework>$(TargetGroup)</TargetFramework>
+    <TargetFramework>$(NuGetTargetMonikerShort)</TargetFramework>


This allows building/restoring the ilproj directly to work (which means you don't have to build all of CoreFX)

eerhardt · 2018-08-09T18:20:23Z

dependencies.props

    </UpdateStep>
-    <UpdateStep Include="BuildTools">
-      <UpdaterType>File</UpdaterType>
-      <Path>$(MSBuildThisFileDirectory)tools-local/ILAsmVersion.txt</Path>


can we delete this file too?

Will do (thought I did already, looks like it came back when I rebased).

tools-local/ILAsmVersion.txt still doesn't look like it is deleted ?

In reply to: 209032299 [](ancestors = 209032299)

tannergooding · 2018-08-09T18:23:35Z

Fixing the mac/linux issue now.

eerhardt

tannergooding · 2018-08-14T16:30:14Z

Directory.Build.props

    <_packageRID Condition="$(TargetGroup.EndsWith('aot'))">win10-$(ArchGroup)-aot</_packageRID>
    <PackageRID Condition="'$(PackageRID)' == ''">$(_packageRID)</PackageRID>
    <PackageRID Condition="'$(PackageRID)' == ''">$(RuntimeOS)-$(ArchGroup)</PackageRID>
+    <MicrosoftNetCoreIlasmPackageRuntimeId Condition="'$(MicrosoftNetCoreIlasmPackageRuntimeId)' == ''">$(PackageRID)</MicrosoftNetCoreIlasmPackageRuntimeId>


Added this, so that the ilasm package we copy from matches what the custom restore targets will use.

tannergooding · 2018-08-15T16:05:14Z

arm is failing because it is a cross-build and PackageRID gets set to linux-arm, which means we are restoring and trying to run the linux-arm binaries for ilasm.

Working on a fix that ensures we restore ilasm for the host rid...

eerhardt · 2018-08-15T16:15:41Z

Just a helpful pointer, this is was the ToolRuntimeRID is for:

corefx/Directory.Build.props

Line 151 in 4f04db9

<ToolRuntimeRID>$(_runtimeOS)-x64</ToolRuntimeRID>

See #21032 if you need more context on the differences in the properties.

joperezr · 2018-08-15T20:22:57Z

external/runtime/runtime.depproj

      <!-- Following is version used in release/uwp6.0 -->
      <Version Condition="'$(TargetGroup)'=='uap10.0.16299'">2.1.0-b-uwp6-25707-02</Version>
    </PackageReference>
+    <PackageReference Include="Microsoft.NETCore.ILAsm">


Does this mean that we will also remove the depproj that restores this today on buildtools? There is a Tools/ilasm/ilasm.depproj today that restores this guy. I ask because with me new changes I will need to restore ildasm and added that new dependency there, but if we are changing it to be here then I'll have to make my change here as well.

It won't be called, for CoreFX, after this change is merged.

I see, in that case I think we should probably remove it from buildtools once this is merged then. Do you mind adding to this same PR one more PackageReference to Microsoft.NETCore.ILDasm? I added that to the buildtools depproj so we will need it here once this is merged, in fact, it is likely build will fail if we don't add that package reference.

We can't remove it from buildtools until after all of CoreCLR is ported to use the msbuild-sdk as well.

I will update the PR so that ILDasm is also referenced, however 😄

weshaggard · 2018-08-21T17:12:02Z

dependencies.props

+      <PackageId>Microsoft.NETCore.ILAsm</PackageId>
+    </XmlUpdateStep>
+    <UpdateStep Include="CoreCLR">
+      <UpdaterType>MSBuildSdk</UpdaterType>


Nice I didn't realize we already had this updater.

We didn't. 😄 @tannergooding added it. dotnet/buildtools#2117

Welcome back, BTW.

Great thanks @tannergooding for adding this support.

weshaggard

Changes look good to me. Thanks for doing this work.

tannergooding · 2018-08-22T20:35:04Z

Directory.Build.props

    <_runtimeOS Condition="'$(_runtimeOS)' == 'tizen.4.0.0'">ubuntu.14.04</_runtimeOS>
    <_runtimeOS Condition="'$(PortableBuild)' == 'true'">$(_portableOS)</_runtimeOS>
    <ToolRuntimeRID>$(_runtimeOS)-x64</ToolRuntimeRID>
+    <MicrosoftNetCoreIlasmPackageRuntimeId Condition="'$(MicrosoftNetCoreIlasmPackageRuntimeId)' == ''">$(ToolRuntimeRID)</MicrosoftNetCoreIlasmPackageRuntimeId>


Updated to use ToolRuntimeRID... Noting that I don't think this works today for building on a 32-bit OS, is that a "known" limitation?

I don't think we have any builds that work on a 32-bit OS.... Do we? The only 32-bit builds that I know of are win-x86 and win-arm (which we build on an x64 Windows machine) and linux-arm (which we build on an x64 Linux machine).

I don't think we have any CI legs, but this was more a question of "Do we support building 32-bit CoreFX from 32-bit Windows?". If we don't, this is probably fine; otherwise, this should probably be updated to be "correct".

I'm not sure if that is supported. @weshaggard ?

I'm not aware of us supporting 32-bit OS's. I think using the ToolRuntimeRID here should be just fine.

tannergooding · 2018-08-22T20:36:07Z

external/tools/tools.depproj

+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), Directory.Build.props))\Directory.Build.props" />
+
+  <PropertyGroup>
+    <NugetRuntimeIdentifier>$(ToolRuntimeRID)</NugetRuntimeIdentifier>


Had to add a new tools.depproj so that Ilasm and Ildasm could be restored cross-plat using the ToolRuntimeRID

Why do you need to restore them with a RID at all? I thought your IL SDK handled that aspect. I was sort of expecting that you simply add the ilproj to the list of projects to restore ahead of time.

Ideally we would do that, but there are a number of "issues" that arise due to the way CoreFX does restore today.
For example, there is no proper intermediate folder for the various *.nuget.g.props/targets and they all get spilled next to the ilproj (or csproj) file.

This is what easily works with the current build system. Calling dotnet restore && dotnet build directly on the ilproj works, but doesn't plugin to the existing build.cmd infrastructure.

OK can you please file a follow-up issue on this to fix once we finish our move to arcade?

tannergooding · 2018-08-22T20:37:21Z

src/System.Runtime.CompilerServices.Unsafe/src/System.Runtime.CompilerServices.Unsafe.ilproj

+    <PackageReference Include="$(MicrosoftNetCoreIldasmPackageName)" Version="$(MicrosoftNetCoreIlasmPackageVersion)" PrivateAssets="all" />
  </ItemGroup>

  <Target Name="AddResourcesFileToIlasm" 


This target is very CoreFX centric and doesn't readily apply to the more general ilproj, so I didn't port it over.

tannergooding · 2018-08-23T17:22:15Z

external/tools/tools.depproj

+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), Directory.Build.props))\Directory.Build.props" />
+
+  <PropertyGroup>
+    <EnableBinPlacing>false</EnableBinPlacing>


Needed to set EnableBinPlacing=false to fix the x86 failures (the x64 coreclr.dll was being binplaced in the test folder otherwise)

tannergooding · 2018-08-24T00:23:12Z

Everything passed last go-around, just had to resolve conflicts.

This should be merge-able first thing tomorrow.

eerhardt · 2018-08-24T19:17:40Z

Directory.Build.props

    <_runtimeOS Condition="'$(_runtimeOS)' == 'tizen.4.0.0'">ubuntu.14.04</_runtimeOS>
    <_runtimeOS Condition="'$(PortableBuild)' == 'true'">$(_portableOS)</_runtimeOS>
    <ToolRuntimeRID>$(_runtimeOS)-x64</ToolRuntimeRID>
+    <MicrosoftNetCoreIlasmPackageRuntimeId Condition="'$(MicrosoftNetCoreIlasmPackageRuntimeId)' == ''">$(ToolRuntimeRID)</MicrosoftNetCoreIlasmPackageRuntimeId>


This property is only used in 1 spot. Does it need to be in the root .props file that everyone imports?

I think we can move it into the ilproj, but I will handle in a follow-up PR to avoid more maestro merge-conflicts.

eerhardt · 2018-08-24T19:18:18Z

external/tools/tools.depproj

+      <Version>$(MicrosoftNetCoreIlasmPackageVersion)</Version>
+    </PackageReference>
+    <PackageReference Include="Microsoft.NETCore.ILDAsm">
+      <Version>$(MicrosoftNetCoreIlasmPackageVersion)</Version>


Is there ever a case where these version numbers will be different? I see we are using the ILAsm version number to get the ILDAsm package.

They should always be the same.

nietras · 2018-10-20T18:03:12Z

@tannergooding can Microsoft.NET.Sdk.IL be used for projects outside of coreclr/corefx? If so how?

AlexeiScherbakov · 2019-01-19T10:15:11Z

@nietras I asked about it here https://github.com/dotnet/coreclr/issues/20816 and answer is here https://github.com/dotnet/coreclr/issues/20816#issuecomment-436427961

tannergooding commented Jul 31, 2018

View reviewed changes

This was referenced Jul 31, 2018

Adding basic support for an ilproj sdk. dotnet/arcade#317

Closed

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

Merged

eerhardt reviewed Jul 31, 2018

View reviewed changes

joshfree added the area-System.Runtime.CompilerServices label Jul 31, 2018

tannergooding mentioned this pull request Aug 7, 2018

Adding an MSBuildSdkUpdater to the BaseDependenciesTask dotnet/buildtools#2117

Merged

tannergooding commented Aug 9, 2018

View reviewed changes

eerhardt reviewed Aug 9, 2018

View reviewed changes

eerhardt approved these changes Aug 9, 2018

View reviewed changes

dagood approved these changes Aug 9, 2018

View reviewed changes

tannergooding commented Aug 14, 2018

View reviewed changes

eerhardt approved these changes Aug 14, 2018

View reviewed changes

tannergooding mentioned this pull request Aug 15, 2018

Adding ILDasm restore and resources switches in order to be able to produce resources for System.Runtime.CompilerServices.Unsafe.dll dotnet/buildtools#2123

Merged

joperezr reviewed Aug 15, 2018

View reviewed changes

weshaggard reviewed Aug 21, 2018

View reviewed changes

weshaggard approved these changes Aug 21, 2018

View reviewed changes

tannergooding commented Aug 22, 2018

View reviewed changes

tannergooding commented Aug 23, 2018

View reviewed changes

karelz assigned tannergooding Aug 23, 2018

Updating S.R.CS.Unsafe to be built using the ilproj sdk.

ad360cd

eerhardt reviewed Aug 24, 2018

View reviewed changes

eerhardt approved these changes Aug 24, 2018

View reviewed changes

tannergooding merged commit 203a9a3 into dotnet:master Aug 24, 2018

karelz added this to the 3.0 milestone Sep 6, 2018

gbalykov mentioned this pull request Oct 2, 2018

Fix Tizen runtime OS #32579

Merged

Updating S.R.CS.Unsafe to be built using the ilproj sdk. #31512

Updating S.R.CS.Unsafe to be built using the ilproj sdk. #31512

Uh oh!

Conversation

tannergooding commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannergooding commented Jul 31, 2018

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!

eerhardt commented Jul 31, 2018

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!

tannergooding commented Aug 9, 2018

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!

tannergooding commented Aug 9, 2018

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding commented Aug 15, 2018

Uh oh!

eerhardt commented Aug 15, 2018

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!

weshaggard 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

tannergooding commented Jul 31, 2018 •

edited

Loading