Add netcoreappaot target group by morganbr · Pull Request #30980 · dotnet/corefx

morganbr · 2018-07-11T04:07:37Z

Adds a netcoreappaot target group intended for CoreRT/.NET Native working with the netcoreapp framework. For the most part, netcoreappaot should be similar to netcoreapp, but it builds against the .NET Native core libraries. A couple of libraries, such as System.Linq.Expressions, have aot-specific definitions that are enabled for netcoreappaot.

I've verified that a couple of netcoreappaot tests pass, but haven't attempted a full test pass locally.

ericstj · 2018-07-11T05:26:14Z

src/shims/ApiCompat.proj

    <_RunApiCompat>true</_RunApiCompat>
    <!-- Disable running apicompat for uap scenarios because the RuntimePath is not correctly setup in BuildAllConfigurations mode -->
-    <_RunApiCompat Condition="'$(BuildAllConfigurations)' == 'true' and $(TargetGroup.StartsWith('uap'))">false</_RunApiCompat>
+    <_RunApiCompat Condition="'$(BuildAllConfigurations)' == 'true' and ($(TargetGroup.StartsWith('uap') or $(PackageTargetRuntimeSuffix)' == 'aot'))">false</_RunApiCompat>


You're missing a trailing parenthesis.

Fixed the paren and quote (I think... not sure how to trigger the error locally)

Build -allconfigurations from the root.

ericstj · 2018-07-11T05:28:00Z

src/System.Runtime/src/System.Runtime.csproj

    <IsPartialFacadeAssembly>true</IsPartialFacadeAssembly>
    <ILLinkClearInitLocals>true</ILLinkClearInitLocals>
-    <GenFacadesIgnoreMissingTypes Condition="'$(TargetGroup)'=='uapaot' OR '$(TargetGroup)' == 'uap'">true</GenFacadesIgnoreMissingTypes>
+    <GenFacadesIgnoreMissingTypes Condition="'$(PackageTargetRuntimeSuffix)'=='aot' OR '$(TargetGroup)' == 'uap'">true</GenFacadesIgnoreMissingTypes>


Let's not use this. Instead can you use TargetsAOT

Lots more like this. Can you make all of them use TargetsAOT? PackageTargetRuntimeSuffix isn't meant to be used like this, its not a configuration property.

Done in all files

ericstj · 2018-07-11T05:28:29Z

src/System.Runtime/ref/System.Runtime.csproj

    <NoWarn>$(NoWarn);0809;0618</NoWarn>
    <!-- Default interfaces are not officially supported in netcoreapp yet -->
-    <DefineConstants Condition="'$(IsPrerelease)' == 'true' and '$(TargetGroup)' == 'netcoreapp'">$(DefineConstants);FEATURE_DEFAULT_INTERFACES</DefineConstants>
+    <DefineConstants Condition="'$(IsPrerelease)' == 'true' and '$(TargetGroup)' == 'netcoreapp' and '$(PackageTargetRuntimeSuffix)'!='aot'">$(DefineConstants);FEATURE_DEFAULT_INTERFACES</DefineConstants>


Please use TargetsAOT

ericstj · 2018-07-11T05:28:45Z

src/System.Runtime.Loader/src/System.Runtime.Loader.csproj

    <AssemblyName>System.Runtime.Loader</AssemblyName>
    <IsPartialFacadeAssembly>true</IsPartialFacadeAssembly>
-    <GenFacadesIgnoreMissingTypes Condition="'$(TargetGroup)'=='uapaot'">true</GenFacadesIgnoreMissingTypes>
+    <GenFacadesIgnoreMissingTypes Condition="'$(PackageTargetRuntimeSuffix)'=='aot'">true</GenFacadesIgnoreMissingTypes>


Please use TargetsAot

ericstj · 2018-07-11T05:29:03Z

src/System.Runtime.InteropServices/src/System.Runtime.InteropServices.csproj

    <IsPartialFacadeAssembly>true</IsPartialFacadeAssembly>
-    <GenFacadesIgnoreMissingTypes Condition="'$(TargetGroup)' == 'uapaot'">true</GenFacadesIgnoreMissingTypes>
-    <NoWarn Condition="'$(TargetGroup)' == 'uapaot'">0436;3001</NoWarn>
+    <GenFacadesIgnoreMissingTypes Condition="'$(PackageTargetRuntimeSuffix)' == 'aot'">true</GenFacadesIgnoreMissingTypes>


Please use TargetsAOT

jkotas · 2018-07-11T05:30:47Z

src/System.Diagnostics.Tools/src/Configurations.props

      uap-Windows_NT;
      uapaot-Windows_NT;
      netcoreapp-Windows_NT;
+      netcoreappaot-Windows_NT;


If we are adding this, we should add it for Unix as well.

I don't think we have a Unix CoreLib in a package we could build that against. If we start producing one somewhere, we could add those flavors.

jkotas · 2018-07-11T05:33:52Z

pkg/Microsoft.Private.CoreFx.NETCoreApp/netcoreapp.rids.props

    <OfficialBuildRID Include="win-x86">
      <Platform>x86</Platform>
    </OfficialBuildRID>
+    <OfficialBuildRID Include="@(OfficialBuildRID->'%(Identity)-aot')" />


Note that this will double the number of official builds.

As we have discussed offline some time ago, I think it would be better to finish refactoring the CoreRT/AOT CoreLib so that we can have one CoreFX build that works for both non-AOT and AOT. It would avoid doubling of the official build flavors.

I checked with @ericstj and this file doesn't actually control official build. I didn't add an official build in this change; I'll do that and CI separately. When I do, I'll just add one more build.

I think it would be better to finish refactoring the CoreRT/AOT CoreLib so that we can have one CoreFX build that works for both non-AOT and AOT.

I think that's a great goal, but when I tried to experiment with using the existing netcoreapp, there were two issues:

There are still lots of differences that would need refactoring.

Testing needs to work differently (running ILC, etc).

By making this change, we can get testing for the runtime and compiler with this framework as well as having all of the remaining differences effectively flagged by the build variables (since this separates aot and uap). If the refactoring is completed, we can look at removing the TargetGroup.

I'm ok with this change. It does mean that the runtime.json inside MS.Private.CoreFx.NetCoreApp will have AOT rids (which may be dangling refs if we don't have official builds) but I am ok with that since its only an internal package to move bits between CoreFx and Core-Setup.

jkotas · 2018-07-11T05:35:32Z

tools-local/targetgroups.props

@@ -168,6 +168,20 @@
      <NuGetTargetMoniker>.NETCoreApp,Version=v1.2</NuGetTargetMoniker>
      <NuGetTargetMonikerShort>netcoreapp1.2</NuGetTargetMonikerShort>
    </TargetGroups>


You can delete the corert Group while you are on this - it was never used.

Similar edits should be in pkg\Microsoft.NETCore.Platforms\runtime.json.

safern · 2018-07-11T23:48:09Z

Directory.Build.props

+    <UseDotNetNativeToolchain Condition="$(_bc_TargetGroup.EndsWith('aot'))">true</UseDotNetNativeToolchain>
    <!-- System.Private.* comes from test ilc, so the symbol packages for those versions will no exist, this is to avoid warnings -->
-    <DownloadCoreCLRSymbols Condition="'$(BuildingUAPAOTVertical)' == 'true'">false</DownloadCoreCLRSymbols>
+    <DownloadCoreCLRSymbols Condition="$(_bc_TargetGroup.EndsWith('aot'))">false</DownloadCoreCLRSymbols>


We can use UseDotNetNativeToolchain here

karelz · 2018-08-15T15:42:24Z

@morganbr do you have ETA for the PR? There was no activity in last 4 weeks.
If you don't have time now, let's close it, until you have the time to finish it. Thanks!

… build.

We decide which RID-specific package to build based on the PackageRID property. Make sure it's set correctly for aot.

… that started to do something similar.

morganbr · 2018-08-17T04:33:07Z

I'm still working on figuring out the allConfigurations issue below. Any hints would certainly be appreciated.

09:12:36 D:\j\workspace\windows-TGrou---0d2c9ac4\Tools\ApiCompat.targets(58,5): error : MembersMustExist : Member 'System.String System.Runtime.CompilerServices.RuntimeFeature.DefaultImplementationsOfInterfaces' does not exist in the implementation but it does exist in the contract. [D:\j\workspace\windows-TGrou---0d2c9ac4\src\System.Runtime\src\System.Runtime.csproj]
09:12:36 D:\j\workspace\windows-TGrou---0d2c9ac4\Tools\ApiCompat.targets(72,5): error : ApiCompat failed for 'D:\j\workspace\windows-TGrou---0d2c9ac4\bin\Windows_NT.AnyCPU.Debug\System.Runtime\netcoreappaot\System.Runtime.dll' [D:\j\workspace\windows-TGrou---0d2c9ac4\src\System.Runtime\src\System.Runtime.csproj]

morganbr · 2018-08-17T05:08:28Z

@ericstj , that failure stems from an incorrect ifdef in the ref project for System.Runtime.csproj.

The define below shouldn't get enabled for netcoreappaot since TargetsAOT should be true, but when using -allConfigurations, it's somehow getting enabled. Any idea why?

    <DefineConstants Condition="'$(IsPrerelease)' == 'true' and '$(TargetGroup)' == 'netcoreapp' and '$(TargetsAOT)'!='true'">$(DefineConstants);FEATURE_DEFAULT_INTERFACES</DefineConstants>

ericstj

Remember aot is a runtime and must satisfy the netcoreapp TFM surface area, same as Linux, osx, etc.

ericstj · 2018-08-17T05:18:42Z

src/System.Runtime/ref/Configurations.props

  <PropertyGroup>
    <BuildConfigurations>
      netcoreapp;
+      netcoreappaot;


You cannot have runtime-specific refs. You must satisfy the netcoreapp ref. Remove this.

Done. This could eventually be problematic since the field in question tells Roslyn that the runtime supports a feature that it doesn't, but I don't think we need to block on it for now.

Runtimes don't have the ability to limit compile-time features. You have to support that feature good enough so that folks can "light-down" at runtime.

The feature in question is default interface implementations. Until that's actually built, I suspect the best we can do is a decent build error. As I said, we don't need to block for now since:

The framework doesn't use them yet, so it shouldn't affect testing

Nobody else uses them yet either since we haven't shipped a CoreCLR with it.

ericstj · 2018-08-17T05:20:34Z

src/System.Runtime/ref/System.Runtime.csproj

    <NoWarn>$(NoWarn);0809;0618</NoWarn>
    <!-- Default interfaces are not officially supported in netcoreapp yet -->
-    <DefineConstants Condition="'$(IsPrerelease)' == 'true' and '$(TargetGroup)' == 'netcoreapp'">$(DefineConstants);FEATURE_DEFAULT_INTERFACES</DefineConstants>
+    <DefineConstants Condition="'$(IsPrerelease)' == 'true' and '$(TargetGroup)' == 'netcoreapp' and '$(TargetsAOT)'!='true'">$(DefineConstants);FEATURE_DEFAULT_INTERFACES</DefineConstants>


Undo and add a baseline to src for missing API

…p and netcoreappaot match.

ericstj · 2018-08-21T16:33:16Z

Test NETFX x86 Release Build
Test Packaging All Configurations x64 Debug Build
https://github.com/dotnet/corefx/issues/31860 was fixed.

morganbr · 2018-08-21T21:31:41Z

Now that the build is clean (and I've verified a test still works), @ericstj, this should be ready to review 😄

ericstj

LGTM. FYI @weshaggard @joperezr

ericstj · 2018-08-21T22:29:50Z

pkg/Microsoft.NETCore.Platforms/runtimeGroups.props

-    <RuntimeGroupWithQualifiers Include="@(RuntimeGroup)">
-      <AdditionalQualifiers>%(AdditionalQualifiers);corert</AdditionalQualifiers>
-    </RuntimeGroupWithQualifiers>
+    <RuntimeGroupWithQualifiers Include="@(RuntimeGroup)" />


We could remove the RuntimeGroupWithQualifiers item entirely and just use RuntimeGroup.

It looks like something gets added further down. Since I don't have insight into how this gets used, I'm not going to do further refactoring in this change.

ericstj · 2018-08-21T22:31:45Z

pkg/Microsoft.Private.CoreFx.NETCoreApp/netcoreapp.rids.props

    <OfficialBuildRID Include="win-x86">
      <Platform>x86</Platform>
    </OfficialBuildRID>
+    <OfficialBuildRID Include="@(OfficialBuildRID->'%(Identity)-aot')" />


I'm ok with this change. It does mean that the runtime.json inside MS.Private.CoreFx.NetCoreApp will have AOT rids (which may be dangling refs if we don't have official builds) but I am ok with that since its only an internal package to move bits between CoreFx and Core-Setup.

morganbr · 2018-08-22T01:18:12Z

Thanks for all the help and reviews, @ericstj!

Changes to add a new netcoreappaot target group, similar to netcoreapp but it builds against the .NET Native core libraries. A couple of libraries, such as System.Linq.Expressions, have aot-specific definitions that are enabled for netcoreappaot. Commit migrated from dotnet/corefx@3333166

morganbr self-assigned this Jul 11, 2018

morganbr requested a review from ericstj July 11, 2018 04:07

morganbr mentioned this pull request Jul 11, 2018

Decouple AoT and UAP testing dotnet/buildtools#2095

Merged

ericstj reviewed Jul 11, 2018

View reviewed changes

jkotas reviewed Jul 11, 2018

View reviewed changes

safern reviewed Jul 11, 2018

View reviewed changes

karelz added this to the 3.0 milestone Jul 19, 2018

morganbr mentioned this pull request Aug 7, 2018

Add WebAssembly build architecture #28936

Merged

karelz added the area-Infrastructure-libraries label Aug 15, 2018

morganbr and others added 14 commits August 16, 2018 00:49

Initial changes to create a netcoreaot target group. A few assemblies…

d9e74b7

… build.

Now builds

07dc190

More build fixes

b541bd7

Redirect netcoreappaot reference paths to netcoreapp

bf66b3c

Fix reference paths

b2bd1e3

Add ApiCompat baselines

31e4745

More packaging progress

21c781e

Fix package build traversal

94722ef

We decide which RID-specific package to build based on the PackageRID property. Make sure it's set correctly for aot.

Permit dangling reference to System.Private.Interop

bf52830

Use aot remote test runner

ea739ab

Testing fixes

aa9516f

Add a netcoreappaot configuration for every assembly that needs it

7d1aaee

Fix XML and Regex to fix build issues

d7d4b85

Cleanup

75430a5

morganbr added 4 commits August 16, 2018 00:49

Fix merge issues

58be9b7

Address code review comments including removing the old corert flavor…

9300521

… that started to do something similar.

Fix merge issues

3076f2e

Fix conflicts

68893ea

ericstj reviewed Aug 17, 2018

View reviewed changes

Fix building allConfigurations by making the references for netcoreap…

83f2cfc

…p and netcoreappaot match.

morganbr force-pushed the AoTPackaging branch from 306198f to 83f2cfc Compare August 21, 2018 04:26

ericstj reviewed Aug 21, 2018

View reviewed changes

ericstj approved these changes Aug 21, 2018

View reviewed changes

morganbr merged commit 3333166 into dotnet:master Aug 22, 2018

filipnavara mentioned this pull request Aug 23, 2018

Move uap, uapaot, netfx, netcoreapp and netcoreapp20 constant definitions to top-level build file. #31470

Merged

Comments

Conversation

morganbr commented Jul 11, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karelz commented Aug 15, 2018

Uh oh!

morganbr commented Aug 17, 2018

Uh oh!

morganbr commented Aug 17, 2018

Uh oh!

ericstj 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!

ericstj commented Aug 21, 2018

Uh oh!

morganbr commented Aug 21, 2018

Uh oh!

ericstj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!