Fix BuildAllConfigurations #15754

ericstj · 2017-02-03T10:27:56Z

This set of commits enables "BuildAllConfigurations" mode.

Best reviewed commit-by-commit.

/cc @weshaggard @mellinoe @tarekgh @joperezr

ericstj · 2017-02-03T13:45:11Z

@stephentoub can you hold off merging package updates until we get this one in? Thanks.

stephentoub · 2017-02-03T13:53:20Z

can you hold off merging package updates until we get this one in?

No problem.

Previously we'd filter the netfx targeting pack when we weren't building for netfx vertical. This was because shims were using all the files in this folder when generating shims to other frameworks (netcoreapp/uap). This breaks in the Build-all scenario. We need the full targeting pack in order to build netfx configurations, but the shims end up getting more files then they need which was causing genfacades to regenerate reference assemblies as facades with typeforwards to themselves. To fix this I stopped filtering when building out the targeting pack, and moved that filtering to the shims.proj.

IsReferenceAssembly has other side-effects, like trimming out any packages that aren't directly referenced. Instead of using this to control binplacing for ref, use BinPlaceRef property directly. This fixes an issue where our netstandard1.x targeting pack folders were missing all but System.Diagnostics.Contracts.

Previously our transform was also applying metadata, which caused MSBuild to batch when creating the item and reorder the items based on that batching (common Configurations were grouped together). To avoid this batching, first apply the metadata, then transform.

Source projects for netcoreapp, uap, and netstandard all require that refs are previously built and binplaced to targeting pack folders. For vertical builds this works correctly because we binplace to the appropriate ref folder for the active BuildConfiguration. For BuildAllConfigurations we need to make sure to binplace for all of these.

This is coming from the runtime package. Including it in both is causing file-in-use issues during build of external.

Previously they were binplacing to the BuildConfiguration-specific RefPath incorrectly. We caught this in BuildAllConfigurations. We were overwriting netcoreapp shims with those from uap.

Make sure that we represent System.PrivateCoreLib as OSGroup specific since it exposes different types on Windows vs Unix.

System.Collections.Immutable uses ExcludeFromCodeCoverageAttribute throughout its source, but this was only added in netstandard2.0. Add a private copy to use in netstandard1.0.

Dataflow requires additional dependencies on netstandard1.x, add them.

There is a source breaking change between ns1.x and ns2.0: old members that return an object[] for GetCustomAttributes have been added back to reflection base types and these are preferred over the extension methods. Avoid this break by ifdefing the code.

ericstj · 2017-02-03T16:43:43Z

/cc @jkotas as well since it looks like he's doing some similar work.

Just to bring everyone up to speed, with this change you can call MSBuild /t:BuildAllConfigurations on any project and it will build all configurations. You still need to have done this at least once for external/dirs.proj and src/ref.builds. I was driving this in to get package build working (since it depends on all configurations for anything that's in a package), but it also satisfies the developer scenario we had before (e.g.: making sure you don't break any of the verticals).

weshaggard · 2017-02-03T16:57:01Z

src/shims/shims.proj

@@ -51,9 +64,9 @@
      <GenFacadesCmd>$(ToolHostCmd) "$(ToolsDir)GenFacades.exe"</GenFacadesCmd>
    </PropertyGroup>

-    <Exec Command="$(GenFacadesCmd) -contracts:&quot;$(NetFxRefPath)&quot; @&quot;$(GenFacadesResponseFile)&quot;" WorkingDirectory="$(ToolRuntimePath)" />


We will likely need push this into the response file, but I will look at that at a later point.

weshaggard · 2017-02-03T16:57:41Z

external/dir.targets

@@ -42,4 +42,7 @@
  <Target Name="GetTargetPath" 
          DependsOnTargets="Compile;GetBinplaceItems" 
          Returns="@(BinplaceItem)" />
+
+  <!-- Don't do any filtering of Targeting pack packages -->
+  <Target Name="FilterTargetingPackResolvedNugetPackages" />


Should we put this in the depproj.targets file?

We can in a follow up.

weshaggard · 2017-02-03T16:58:50Z

external/netfx/netfx.depproj

-
-  <!-- Filter the targeting pack to just these assemblies which we need netcoreapp shims for -->
-  <ItemGroup>
-    <TargetingPackReference Include="mscorlib" />


I'm interested in understanding why the filtered approach didn't work still. My expectation is that our builds didn't need the full targeting pack and would likely be able to get away with just a smaller subset.

We can filter what goes into netfx. Previously I had a separate filter for netfx/net4* configured projects and shims. This broke because they needed to use the same folder. The shims will always be a smaller subset than the libs we build against so we need filtering in the shims. We can easily add back filtering to the depprojs at will in the future.

tarekgh · 2017-02-03T16:58:53Z

src/Microsoft.Win32.Registry.AccessControl/src/Microsoft.Win32.Registry.AccessControl.csproj

@@ -23,13 +23,13 @@
    </Compile>
  </ItemGroup>
  <ItemGroup Condition="'$(TargetGroup)' == 'netfx'">
-    <Compile Include="Microsoft\Win32\RegistryAclExtensions.netfx.cs" />
+    <Compile Include="Microsoft\Win32\RegistryAclExtensions.net46.cs" />


why renamed this one?

I previously broke it.

weshaggard · 2017-02-03T17:02:09Z

binplace.targets

@@ -2,7 +2,7 @@
 <Project ToolsVersion="14.0" InitialTargets="CheckForBuildTools" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

  <PropertyGroup>
-    <IsRuntimeAssembly Condition="'$(IsRuntimeAssembly)'=='' AND '$(IsReferenceAssembly)' != 'true' AND '$(IsTestProject)' != 'true'">true</IsRuntimeAssembly>
+    <IsRuntimeAssembly Condition="'$(IsRuntimeAssembly)'=='' AND '$(IsReferenceAssembly)' != 'true' AND '$(BinPlaceRef)' != 'true' AND '$(IsTestProject)' != 'true'">true</IsRuntimeAssembly>


Why does BinPlaceRef != true equate to not a runtime assembly? I thought we do have some cases where it would be a runtime assembly that we bin place into ref.

See below. We're only here if a project only declared BinPlaceRef (and not BinPlaceRuntime). This doesn't break anything, it allows for projects to opt in to only ref without getting side effects of claiming to be IsReferenceAssembly.

weshaggard · 2017-02-03T17:03:02Z

external/netfx/netfx.depproj

@@ -2,7 +2,7 @@
 <Project ToolsVersion="14.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003" DefaultTargets="Build">
  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
  <PropertyGroup>
-    <IsReferenceAssembly>true</IsReferenceAssembly>
+    <BinPlaceRef>true</BinPlaceRef>


Note this change will conflict with my PR #15747. We need to make sure one of us gets this in correctly.

This doesn't actually conflict. IsReferenceAssembly still works, but it triggers the "OmitTransitiveCompileReferences" behavior which was trimming out all of NETStandard.Library. For the TargetingPack packages they are flat so they wouldn't hit this. We should try to be consistent about how we specify this, but that can be cleanup, not conflict.

Yep I'm cleaning that up with my current PR.

tarekgh · 2017-02-03T17:03:02Z

src/System.Threading/src/System.Threading.csproj

@@ -6,7 +6,7 @@
    <AssemblyName>System.Threading</AssemblyName>
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <IsPartialFacadeAssembly>true</IsPartialFacadeAssembly>
-    <GenFacadesArgs Condition="'$(TargetGroup)'=='uapaot'">$(GenFacadesArgs) -ignoreMissingTypes</GenFacadesArgs>
+    <GenFacadesIgnoreMissingTypes Condition="'$(TargetGroup)'=='uapaot'">true</GenFacadesIgnoreMissingTypes>


where GenFacadesIgnoreMissingTypes get used?

In the task that runs genfacades. @mellinoe broke this when converting from command to task. Task understands this property but not args.

My bad. I had rebased my old branch and so I missed projects that started using GenFacadesArgs since my branch was created.

tarekgh · 2017-02-03T17:04:13Z

src/System.IO.Ports/src/Configurations.props

@@ -3,7 +3,7 @@
  <PropertyGroup>
    <BuildConfigurations>
      netstandard-Windows_NT;
-      net463-AnyOS;
+      netfx;


should we add the OS here? I mean netfx-Windows_NT?

Discussed this with Wes in a previous PR. Its not required.

weshaggard · 2017-02-03T17:05:09Z

binplace.targets

    <BinplaceConfiguration Include="netstandard-$(OSGroup)">
      <RefPath>$(RefRootPath)netstandard</RefPath>
    </BinplaceConfiguration>
+    <BinplaceConfiguration Condition="'$(BuildAllConfigurations)' == 'true' AND '$(_bc_TargetGroup)' != 'netcoreapp'" 


When doing BuildAllConfigurations would the BuildConfiguration almost always be the default one?

tarekgh · 2017-02-03T17:07:14Z

Added some questions but other than that LGTM

weshaggard · 2017-02-03T17:09:45Z

external/runtime/runtime.depproj

@@ -2,6 +2,9 @@
 <Project ToolsVersion="14.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003" DefaultTargets="Build">
  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
  <PropertyGroup>
+    <!-- support cross-targeting by choosing a RID to restore when running on a different machine that what we're build for -->
+    <NugetRuntimeIdentifier Condition="'$(OSGroup)' == 'Windows_NT' AND '$(OSEnvironment)' != 'Windows_NT'">win7-x64</NugetRuntimeIdentifier>


OSEnvironment as recently been removed. #15728, Use RunningOnUnix property for these 2 conditions.

weshaggard · 2017-02-03T17:15:04Z

src/System.IO.Packaging/ref/Configurations.props

@@ -2,6 +2,7 @@
 <Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>
    <BuildConfigurations>
+      netstandard1.3;


Just so I understand your logic because in some cases you are adding older versioned netstandard configs and in other cases you are removing them. Are you adding them when you think there is actual implementation we still want to build live for the package and removing them when they are just facades and not interesting to build live?

Yes, adding when I think we'll want to ship that live. Removing when we'd be ok with harvesting.

weshaggard · 2017-02-03T17:28:32Z

src/Microsoft.CSharp/Microsoft.CSharp.sln

@@ -2,17 +2,17 @@ Microsoft Visual Studio Solution File, Format Version 12.00
 # Visual Studio 14
 VisualStudioVersion = 14.0.25420.1
 MinimumVisualStudioVersion = 10.0.40219.1
-Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Microsoft.CSharp.Tests", "tests\Microsoft.CSharp.Tests.csproj", "{82B54697-0251-47A1-8546-FC507D0F3B08}"


Did you run this update on your Mac? If so we should probably normalize the slashes in our task to avoid these noop updates. This will definitely cause VS to rewrite all these solution files.

Probably :/

jkotas · 2017-02-03T17:30:16Z

src/System.Collections/src/Configurations.props

@@ -1,9 +1,9 @@
 <?xml version="1.0" encoding="utf-8"?>
 <Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>
+    <!-- Configurations not yet building netcoreapp1.2corert; -->


I do not think you need to bother commenting on netcoreapp1.2corert config missing. It is heavily bit-rotten and it would be best to redo it from scratch.

ericstj · 2017-02-03T19:01:07Z

Merging this to avoid conflicts and will follow up to address feedback (which is less conflict prone).

weshaggard · 2017-02-03T19:10:12Z

@ericstj I think this merge might break a couple things. I'm pulling it down and verifying and will fix any remaining issues. Go enjoy your time-off.

ericstj · 2017-02-03T19:14:17Z

I've also got packaging working that builds on top of this. Hoping to get that PR up so that folks can work on it as need be.

Let me know what you think will be broken and I can try to address. On in-flight wifi to try and wrap all this up.

weshaggard · 2017-02-03T19:24:38Z

Turns out it didn't break #15747 but I do need to update winrt.depproj to use BinPlaceRef and your changes did conflict with #15765 but I will merge with your changes and fix the other issue with that PR.

Address feedback from #15754

Fix BuildAllConfigurations Commit migrated from dotnet/corefx@0620fb0

Address feedback from dotnet/corefx#15754 Commit migrated from dotnet/corefx@6424a61

dnfclas added the cla-already-signed label Feb 3, 2017

ericstj added 17 commits February 3, 2017 08:30

Remove CoreCLR from test-runtime

fb30574

This is coming from the runtime package. Including it in both is causing file-in-use issues during build of external.

Shims should binplace to config-specific RefPath

843cc24

Previously they were binplacing to the BuildConfiguration-specific RefPath incorrectly. We caught this in BuildAllConfigurations. We were overwriting netcoreapp shims with those from uap.

System.Private.CoreLib is OSGroup-specific

d549c08

Make sure that we represent System.PrivateCoreLib as OSGroup specific since it exposes different types on Windows vs Unix.

Use a private ExcludeFromCodeCoverageAttribute

d676bca

System.Collections.Immutable uses ExcludeFromCodeCoverageAttribute throughout its source, but this was only added in netstandard2.0. Add a private copy to use in netstandard1.0.

Fix Dataflow build

e12a443

Dataflow requires additional dependencies on netstandard1.x, add them.

Remove configurations that aren't building

eef6073

Fix legacy build configurations

48680c2

Fix missing doc comment on Span

3010d7d

Fix uses of GenFacadesArgs -ignoreMissingTypes

ac5b1e7

Fix desktop build of Ports

814253c

Fix System.Memory ns1.0 build

42844e6

Fix legacy builds of RuntimeInformation

a2f3d67

ericstj force-pushed the buildAllFixes branch from faafbe7 to a2f3d67 Compare February 3, 2017 16:32

Sync BuildConfiguration with project PropertyGroups

758a0e1