Port GenFacades, Microsft.Cci.Extensions to Arcade #477

wtgodbe · 2018-08-16T22:56:09Z

This ports GenFacades & Microsft.Cci.Extensions to Arcade. It does the following things:

Removes GenFacades.Console in favor of GenFacades.Core
Removes partialfacades.exe.targets in favor of partialfacades.task.targets
Folds partialfacades.task.targets into partialfacades.targets
Moves GenFacadesTask.cs to GenFacades.Core
Renames GenFacades.Core to Microsoft.DotNet.GenFacades
Creates a package for Microsoft.DotNet.GenFacades which contains Microsoft.Cci.Extensions

@joperezr @jcagme @chcosta @tmat @weshaggard PTAL

I'd also love ideas on the best way to test this package out in CoreFx.

wtgodbe · 2018-08-16T22:57:41Z

The most interesting files are the .csproj/.targets files, and maybe GenFacadesTask.cs, as well as the general layout/naming scheme I've chosen (which is all described in the PR description)

wtgodbe · 2018-08-17T18:04:34Z

error : Zip Container 'packages\NonShipping\Microsoft.DotNet.GenFacades.1.0.0-ci.nupkg' has part 'tools/netstandard2.0/Microsoft.Cci.dll' which is not listed in the sign or external list

@jcagme do you know what list I need to edit here?

joperezr · 2018-08-17T18:38:32Z

@jcagme do you know what list I need to edit here?

eng/SignToolData.json

jcagme · 2018-08-17T18:43:23Z

Is the plan for this to end up in myget? If yes then go ahead and add it to eng/SignToolData.json if not we should have a conversation

wtgodbe · 2018-08-17T18:50:07Z

Yes, the plan is for it to wind up in myget.

src/Microsoft.DotNet.Cci.Extensions/Microsoft.DotNet.Cci.Extensions.csproj

@@ -0,0 +1,16 @@
+<Project Sdk="Microsoft.NET.Sdk">


src/Microsoft.DotNet.Cci.Extensions/Microsoft.DotNet.Cci.Extensions.csproj

+    <PackageReference Include="Microsoft.Composition" Version="1.0.30" />
+    <PackageReference Include="System.Diagnostics.Contracts" Version="4.0.0" />
+    <PackageReference Include="System.Diagnostics.TraceSource" Version="4.0.0" />
+    <PackageReference Include="NETStandard.Library" Version="1.6.0" />


src/Microsoft.DotNet.Cci.Extensions/Microsoft.DotNet.Cci.Extensions.csproj

@@ -0,0 +1,16 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <TargetFrameworks>net461</TargetFrameworks>


weshaggard · 2018-08-22T20:30:19Z

I'd also love ideas on the best way to test this package out in CoreFx.

You likely need to hack the targets locally to reference your targets instead of the ones in Buildtools.

chcosta · 2018-08-23T16:29:39Z

fyi @StephenBonikowsky

src/Microsoft.Cci.Extensions/Microsoft.Cci.Extensions.csproj

+  </PropertyGroup>
+
+  <PropertyGroup>
+    <NoWarn>NU1701</NoWarn>


eng/SignToolData.json

@@ -4,12 +4,15 @@
      "certificate": "MicrosoftSHA2",
      "strongName": "MsSharedLib72",
      "values": [
+        "bin/Microsoft.Cci.Extensions/net461/Microsoft.Cci.Extensions.dll",


wtgodbe · 2018-08-23T20:18:33Z

Updated to address feedback. Working on local CoreFx testing now.

JohnTortugo · 2018-08-23T20:38:42Z

I think it was monday. Take a look at PR #410

…

On Thu, Aug 23, 2018, 13:24 William Godbe, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In eng/SignToolData.json <#477 (comment)>: > @@ -4,12 +4,15 @@ "certificate": "MicrosoftSHA2", "strongName": "MsSharedLib72", "values": [ + "bin/Microsoft.Cci.Extensions/net461/Microsoft.Cci.Extensions.dll", Oh, awesome! How recent is that change? I was getting errors last week from not having .dll's in this file — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#477 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACJTsG3eS_lQ6O4tbplKlrXmVo_GsRMWks5uTw-PgaJpZM4WAtFE> .

src/Microsoft.Cci.Extensions/Microsoft.Cci.Extensions.csproj

@@ -1,16 +1,20 @@
 <Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
-    <TargetFrameworks>net461</TargetFrameworks>
+    <TargetFrameworks>net461;netstandard2.0</TargetFrameworks>


weshaggard · 2018-08-23T22:18:46Z

Removes partialfacades.exe.targets in favor of partialfacades.task.targets

Why did you remove the exe support? I'm unsure if there are any existing cases for the exe but it does provide some level of flexibility.

wtgodbe · 2018-08-23T22:28:11Z

@weshaggard I didn't believe there were any remaining uses of it. I'm not opposed to adding it back if we want to keep the functionality just in case.

wtgodbe · 2018-08-23T22:28:32Z

Currently getting the following when building with this in CoreFx, I'm investigating:

E:\code\corefx\packages\microsoft.dotnet.genfacades\1.0.0-dev\PackageFiles\partialfacades.targets(97,5): error : Arithmetic operation resulted in an overflow. [E:\code\corefx\src\System.Threading.Tasks.Extensions\ref\System.Threading.Tasks.Extensions.csproj]

weshaggard · 2018-08-23T22:50:04Z

@weshaggard I didn't believe there were any remaining uses of it. I'm not opposed to adding it back if we want to keep the functionality just in case.

I'm ok excluding the exe version for now to make things easier, if we find we need it we can add it.

src/Microsoft.DotNet.GenFacades/PackageFiles/notsupported.targets

+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="12.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+
+  <PropertyGroup Condition="'$(GeneratePlatformNotSupportedAssembly)' == 'true' OR '$(GeneratePlatformNotSupportedAssemblyMessage)' != ''">


wtgodbe · 2018-08-29T23:01:58Z

The error is happening here:

arcade/src/Microsoft.DotNet.GenFacades/VersionResourceSerializer.cs

Line 98 in 718dd8d

    
           writer.Write((DWORD)(productVersion.Build << 16) | (uint)productVersion.Revision);

wtgodbe · 2018-08-31T17:10:20Z

productVersion.Build is equal to -1 here, which causes the overflow when we try to do productVersion.Build << 16. Could just be due to Arcade's versioning.

wtgodbe · 2018-09-04T19:31:34Z

This happens on System.Threading.Tasks.Extensions in CoreFx, which has a _productVersionContents of "4.6.26823.0 @BuiltBy: wigodbe-WILLPC @Branch: master @SrcCode: https://github.com/dotnet/corefx/tree/e1a33f4a1cc582f42bd5ae0f887bf1aa9878d493". We then call Version.TryParse() on that value at

arcade/src/Microsoft.DotNet.GenFacades/VersionResourceSerializer.cs

Line 90 in 718dd8d

if (!Version.TryParse(_productVersionContents, out productVersion))

.

We then go into https://github.com/dotnet/corefx/blob/master/src/Common/src/CoreLib/System/Version.cs#L322-L391. We then find the indices of the three periods within the version string, but then we hit https://github.com/dotnet/corefx/blob/master/src/Common/src/CoreLib/System/Version.cs#L343. Since the __productVersionContents contains additional periods, we return null. Therefore, ProductVersion is set to a new Version(0, 0) -

arcade/src/Microsoft.DotNet.GenFacades/VersionResourceSerializer.cs

Line 91 in 718dd8d

productVersion = new Version(0, 0);

. In Version.cs, Build and Revision are initialized to -1 unless otherwise set: https://github.com/dotnet/corefx/blob/master/src/Common/src/CoreLib/System/Version.cs#L23-L24. Which causes our exception when we try to left-shift productVersion.Build by 16 at

arcade/src/Microsoft.DotNet.GenFacades/VersionResourceSerializer.cs

Line 98 in 718dd8d

    
           writer.Write((DWORD)(productVersion.Build << 16) | (uint)productVersion.Revision);

. I'm not sure why this isn't happening in normal builds of corefx - looking now.

wtgodbe · 2018-09-07T22:05:36Z

In the current state, shims.proj fails when calling GenFacades.exe. Debugging.

wtgodbe · 2018-09-07T22:46:46Z

Looks like I'm missing hostpolicy.dll, which doesn't get packaged with GenFacades.exe, or placed into its bin dir. I borrowed hostpolicy.dll from another location in Arcade & placed it into CoreFx's Tools dir, and got Could not resolve CoreCLR path. Detail dump below:

Using E:\code\corefx\Tools\Microsoft.DotNet.GenFacades.Console.deps.json deps file
Could not locate the dependencies manifest file [E:\code\corefx\Tools\Microsoft.DotNet.GenFacades.Console.deps.json]. Some libraries may fail to resolve.

There is no Microsoft.DotNet.GenFacades.Console.deps.json built in Arcade.

Some earlier (unrelated) command during build was able to successfully set the CoreClr location to E:\code\corefx\Tools\dotnetcli\shared\Microsoft.NETCore.App\2.1.3-servicing-26708-02.

If I place the contents of that folder into Tools, the execution of GenFacades.exe works, but then I get a string of APICompat failures:

E:\code\corefx\Tools\ApiCompat.targets(72,5): error : ApiCompat failed for 'E:\code\corefx\bin\Windows_NT.AnyCPU.Debug\System.Security.Principal.Windows\netcoreapp\System.Security.Principal.Windows.dll' [E:\code\corefx\src\System.Security.Principal.Windows\src\System.Security.Principal.Windows.csproj]
Unhandled Exception: System.IO.FileLoadException: Could not load file or assembly 'Microsoft.Cci.Extensions, Version=999.999.999.999, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)

weshaggard · 2018-09-07T23:04:13Z

Looks like I'm missing hostpolicy.dll, which doesn't get packaged with GenFacades.exe, or placed into its bin dir. I borrowed hostpolicy.dll from another location in Arcade & placed it into CoreFx's Tools dir, and got Could not resolve CoreCLR path.

hostpolicy isn't supposed to come with the exe. This is just an indication that you are missing the runtime you are trying to run on. What is your runtimeconfig file say is the runtime you are trying to run on? As any asside we currently override the runtimeconfig file as part of init-tools.sh (https://github.com/dotnet/buildtools/blob/master/src/Microsoft.DotNet.Build.Tasks/PackageFiles/init-tools.ps1#L11) so we might need to figure out another solution. Perhaps a roll-forward policy.

wtgodbe · 2018-09-07T23:10:41Z

I don't see a runtimeconfig for GenFacades in Tools or in Arcade

weshaggard · 2018-09-07T23:35:45Z

I don't see a runtimeconfig for GenFacades in Tools or in Arcade

There is definitely a GenFacades.runtimeconfig.json file in Tools directory today. It gets generated as part of the build if there isn't one present. We may need to play with it a little to see what our best option is.

wtgodbe · 2018-09-12T20:20:14Z

Working on converting the call to the .exe in shims.proj to a call to the task. Currently hitting the arithmetic overflow again (only in shims, strangely). Debugging now.

wtgodbe · 2018-09-12T20:45:53Z

I was able to get this working with CoreFx with a few minor tweaks. For one, shims.proj now looks like this: https://github.com/wtgodbe/corefx/blob/Shims/src/shims/shims.proj

We will probably also need a change to https://github.com/dotnet/buildtools/blob/master/src/Microsoft.DotNet.Build.Tasks/PackageFiles/Build.Common.targets#L220, perhaps adding a property called PartialFacadesFile that defaults to $(MSBuildThisFileDirectory)partialfacades.targets. We could then overwrite that property in CoreFx with the location of the version of the file restored from Arcade.

Also, we'll probably want to wait until we port GenApi/ApiCompat (which I'm going to do next) before consuming this in CoreFx, since otherwise the version of Microsoft.Cci.Extensions that GenFacades depends on will conflict with the version that ApiCompat depends on (I had to resolve this by keeping both versions of the .dll in 2 separate locations when I was hacking CoreFx together).

@weshaggard is there anything else I need to address here?

src/Microsoft.DotNet.GenFacades/VersionResourceSerializer.cs

-            if (!Version.TryParse(_fileVersionContents, out fileVersion))
+            // For the File & Product Version, we take the first portion of the string before any whitespace,
+            // in case there is extraneous information (e.g. '@BuiltBy') after the version number.
+            if (!Version.TryParse(_fileVersionContents.Split(null)[0], out fileVersion))


src/Microsoft.DotNet.GenFacades/Microsoft.DotNet.GenFacades.csproj

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


weshaggard

Other then the last couple comments the change looks good overall. I'm fine with you getting this in and then making any necessary changes to start consuming it in corefx as a follow-up.

wtgodbe added the * NO MERGE * label Aug 16, 2018

wtgodbe requested review from tmat, weshaggard, chcosta, joperezr and jcagme August 16, 2018 22:56

wtgodbe force-pushed the GenFacade branch from e45c819 to 56cc679 Compare August 17, 2018 21:30

weshaggard reviewed Aug 22, 2018

View reviewed changes

src/Microsoft.DotNet.Cci.Extensions/Microsoft.DotNet.Cci.Extensions.csproj Outdated

@@ -0,0 +1,16 @@

<Project Sdk="Microsoft.NET.Sdk">

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

weshaggard reviewed Aug 22, 2018

View reviewed changes

src/Microsoft.DotNet.Cci.Extensions/Microsoft.DotNet.Cci.Extensions.csproj Outdated

@@ -0,0 +1,16 @@

<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>

<TargetFrameworks>net461</TargetFrameworks>

This comment was marked as spam.

Sign in to view

wtgodbe commented Aug 23, 2018

View reviewed changes

src/Microsoft.Cci.Extensions/Microsoft.Cci.Extensions.csproj

</PropertyGroup>

<PropertyGroup>

<NoWarn>NU1701</NoWarn>

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

JohnTortugo reviewed Aug 23, 2018

View reviewed changes

weshaggard reviewed Aug 23, 2018

View reviewed changes

src/Microsoft.Cci.Extensions/Microsoft.Cci.Extensions.csproj Outdated

@@ -1,16 +1,20 @@

<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>

<TargetFrameworks>net461</TargetFrameworks>

<TargetFrameworks>net461;netstandard2.0</TargetFrameworks>

This comment was marked as spam.

Sign in to view

weshaggard reviewed Aug 27, 2018

View reviewed changes

wtgodbe added 10 commits September 7, 2018 14:13

Change csproj name

7b72abd

use Microsoft.Cci where needed & add to SignToolData

636446f

Microsoft.DotNet.Cci.Extensions -> Microsoft.Cci.Extensions

9ab0514

Add netstandard2.0 to cci.extensions

0f9cbbd

Spacing

3e8676a

Fix mismatched property tag

85b09b8

Add notsupported.targets & remove NoWarn from .csproj

26021d0

Remove dnxcore50, add NoWarn back

f499ba7

Remove net461 & Disable Implicit AssetTargetFallback

18bef65

Bump System.Diag.Contracts to 4.3.0

f342796

wtgodbe force-pushed the GenFacade branch from 207cb4a to 9866f20 Compare September 7, 2018 21:16

Don't port GenFacades.exe, fix overflow

0222191

wtgodbe force-pushed the GenFacade branch from 00bef15 to 0222191 Compare September 12, 2018 19:58

Add overflow fix to FileVersion as well

520985b

wtgodbe added 2 commits September 12, 2018 13:49

Resolve conflicts

ab4f646

Merge branch 'master' into GenFacade

d2b5243

weshaggard reviewed Sep 12, 2018

View reviewed changes

src/Microsoft.DotNet.GenFacades/Microsoft.DotNet.GenFacades.csproj Outdated

<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>

<TargetFrameworks>netstandard2.0</TargetFrameworks>

This comment was marked as spam.

Sign in to view

weshaggard approved these changes Sep 12, 2018

View reviewed changes

Address feedback

b49f50c

wtgodbe merged commit 04f4b34 into dotnet:master Sep 12, 2018

wtgodbe removed the * NO MERGE * label Sep 12, 2018

wtgodbe changed the title ~~[WIP] Port GenFacades, Microsft.Cci.Extensions to Arcade~~ Port GenFacades, Microsft.Cci.Extensions to Arcade Sep 12, 2018

Port GenFacades, Microsft.Cci.Extensions to Arcade #477

Port GenFacades, Microsft.Cci.Extensions to Arcade #477

Conversation

wtgodbe commented Aug 16, 2018 • edited Loading

wtgodbe commented Aug 16, 2018

wtgodbe commented Aug 17, 2018

joperezr commented Aug 17, 2018

jcagme commented Aug 17, 2018

wtgodbe commented Aug 17, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

weshaggard commented Aug 22, 2018

chcosta commented Aug 23, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

wtgodbe commented Aug 23, 2018

JohnTortugo commented Aug 23, 2018 via email

This comment was marked as spam.

weshaggard commented Aug 23, 2018

wtgodbe commented Aug 23, 2018

wtgodbe commented Aug 23, 2018

weshaggard commented Aug 23, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

wtgodbe commented Aug 29, 2018

wtgodbe commented Aug 31, 2018

wtgodbe commented Sep 4, 2018

wtgodbe commented Sep 7, 2018

wtgodbe commented Sep 7, 2018 • edited Loading

weshaggard commented Sep 7, 2018

wtgodbe commented Sep 7, 2018

weshaggard commented Sep 7, 2018

wtgodbe commented Sep 12, 2018

wtgodbe commented Sep 12, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

weshaggard left a comment

Choose a reason for hiding this comment

wtgodbe commented Aug 16, 2018 •

edited

Loading

wtgodbe commented Sep 7, 2018 •

edited

Loading