Re-enable UAP builds #27531

nattress · 2018-02-28T00:23:03Z

Revert the three commits that initially disabled the build
Add a uapaot flavor for System.Numerics.Vectors. It can no longer use the uap flavor since it depends directly on System.Private.CoreLib (whose strong name key is different for uap and uapaot).
Fix a series of UAP build breaks that have crept in over the last couple weeks

Fixes https://github.com/dotnet/corefx/issues/26802

nattress · 2018-02-28T00:29:32Z

@jkotas PTAL at 80b0751
@stephentoub PTAL at 30d10f5
@JeffCyr / @stephentoub PTAL at f3a86db

If any of you can sanity check the other commits that would be great, too. Thanks!

jkotas · 2018-02-28T00:37:19Z

If we re-enable this, what is our plan going to be with getting .NET Core API changes through? cc @joshfree

jkotas · 2018-02-28T00:39:30Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.Uap.cs

+{
+    internal static partial class NameResolutionPal
+    {
+


Nit: extra lines

ahsonkhan · 2018-02-28T05:30:33Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.NetCoreApp.cs

+                    return false;
+
+                // We can't just check that 'GetAddrInfoEx' exists, because it existed before supporting overlapped.
+                // The existance of 'GetAddrInfoExCancel' indicates that overlapped is supported.


nit: typo existance

ahsonkhan · 2018-02-28T05:31:20Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.NetCoreApp.cs

+{
+    internal static partial class NameResolutionPal
+    {
+


nit: extra line

ahsonkhan · 2018-02-28T05:32:29Z

src/Common/src/System/IO/PathInternal.Windows.cs

+        /// Adds the extended path prefix (\\?\) if not already a device path, IF the path is not relative,
+        /// AND the path is more than 259 characters. (> MAX_PATH + null)
+        /// </summary>
+        internal static string EnsureExtendedPrefixOverMaxPath(string path)


cc @JeremyKuhne, PTAL

JeremyKuhne · 2018-02-28T06:38:08Z

src/Common/src/System/IO/PathInternal.Windows.cs

+        /// Adds the extended path prefix (\\?\) if not already a device path, IF the path is not relative,
+        /// AND the path is more than 259 characters. (> MAX_PATH + null)
+        /// </summary>
+        internal static string EnsureExtendedPrefixOverMaxPath(string path)


What API did this get added for? I specifically updated to do this below to put the prefix on with trailing spaces & periods to fix handling those paths.

C:\git\corefx\src\Common\src\Interop\Windows\kernel32\Interop.CreateFile2.cs(95,39): error CS0117: 'PathInternal' does not contain a definition for 'EnsureExtendedPrefixOverMaxPath' [C:\git\corefx\src\System.IO.FileSystem.Watcher\src\System.IO.FileSystem.Watcher.csproj]

Thanks- please change that to the method below and remove this method.

@JeremyKuhne Ok, so to confirm, I'll replace the call to EnsureExtendedPrefixOverMaxPath in Interop.CreateFile2.cs with a call to EnsureExtendedPrefixIfNeeded ?

JeremyKuhne

Need to know what the EnsureExtendedPrefixOverMaxPath got added for to make sure we're not regressing a fix.

jkotas · 2018-02-28T19:27:01Z

src/System.Net.NameResolution/src/System.Net.NameResolution.csproj

@@ -54,6 +54,8 @@
  </ItemGroup>
  <ItemGroup Condition=" '$(TargetsWindows)' == 'true'">
    <Compile Include="System\Net\NameResolutionPal.Windows.cs" />
+    <Compile Include="System\Net\NameResolutionPal.Windows.NetCoreApp.cs" Condition="'$(TargetGroup)' != 'uap'"/>


We typically use Win32.cs suffix for these.

So .Win32.cs and .Uap.cs suffixes then?

I think so. Here is example of prior art:

ProcessStartInfo.Uap.cs ProcessStartInfo.Unix.cs ProcessStartInfo.Win32.cs ProcessStartInfo.Windows.cs

jkotas · 2018-02-28T19:28:49Z

src/System.Net.NameResolution/src/System.Net.NameResolution.csproj

@@ -132,7 +134,7 @@
    <Compile Include="$(CommonPath)\Interop\Windows\Kernel32\Interop.GetProcAddress.cs">


Exclude this one for Uap as well?

jkotas · 2018-03-01T00:26:37Z

src/System.Net.NameResolution/src/System.Net.NameResolution.csproj

@@ -54,6 +54,8 @@
  </ItemGroup>
  <ItemGroup Condition=" '$(TargetsWindows)' == 'true'">
    <Compile Include="System\Net\NameResolutionPal.Windows.cs" />
+    <Compile Include="System\Net\NameResolutionPal.Windows.Win32.cs" Condition="'$(TargetGroup)' != 'uap'"/>


Nit: There is no file with Windows.Win32.cs suffix today. Everything is using just .Win32.cs.

jkotas · 2018-03-01T00:27:31Z

src/System.Runtime.Extensions/src/System/Globalization/Extensions.cs

@@ -0,0 +1,96 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.


Can we just add this to API compat baseline instead of adding the whole implementation back?

This failure is in GenFacades, not APICompat. So I believe not?

<GenFacadesIgnoreMissingTypes Condition="'$(TargetGroup)'=='uapaot'">true</GenFacadesIgnoreMissingTypes>

Used in number of places already.

Perfect - thanks!

jkotas · 2018-03-01T00:27:55Z

src/System.Threading.ThreadPool/ref/System.Threading.ThreadPool.cs

@@ -23,7 +23,11 @@ public static partial class ThreadPool
        public static void GetMinThreads(out int workerThreads, out int completionPortThreads) { throw null; }
        public static bool QueueUserWorkItem(System.Threading.WaitCallback callBack) { throw null; }
        public static bool QueueUserWorkItem(System.Threading.WaitCallback callBack, object state) { throw null; }
+#if uap


Just add it to the API compat baseline instead of adding ifdefs?

Yep that worked great.

jkotas · 2018-03-01T00:39:09Z

src/System.Net.Http/ref/System.Net.Http.csproj

@@ -10,6 +10,7 @@
  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'uap-Release|AnyCPU'" />
  <ItemGroup>
    <Compile Include="System.Net.Http.cs" />
+    <Compile Include="System.Net.Http.Win32.cs" Condition="'$(TargetGroup)' != 'uap'" />


This is not Win32.cs specific. It is for both Windows and Unix. I think the original name would be fine. Or ifdef in the ref file would be fine too, I think. We have multiple other places with itdefs in ref files.

I'll switch this to be #if uap

jkotas · 2018-03-01T00:40:08Z

src/System.Net.Http/src/System.Net.Http.csproj

@@ -214,6 +215,7 @@
  </PropertyGroup>
  <ItemGroup Condition=" '$(TargetsUnix)' == 'true' ">
    <Compile Include="System\Net\Http\HttpClientHandler.Core.cs" />
+    <Compile Include="System\Net\Http\HttpClientHandler.Win32.cs" />


It looks weird to be including Win32.cs file for Unix...

Ok I'm going to just go with .NetCoreApp.cs for this. We already have .Unix, .Windows, .Core.

Nit: It should be all lowercase netcoreapp.cs to match the convention

JeffCyr · 2018-03-01T14:12:21Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.Uap.cs

+{
+    internal static partial class NameResolutionPal
+    {
+        private static bool GetAddrInfoExSupportsOverlapped() => false;


GetAddrInfoEx and GetAddrInfoExCancel are available in UAP no? LoadLibraryEx was used to test for unsupported windows version, but this may not be needed for UAP and it could return true.

https://docs.microsoft.com/en-us/uwp/win32-and-com/win32-apis

Yeah I think you're right. Once I get this checked in I'll verify and flip to true.

danmoseley · 2018-03-01T18:01:28Z

Do you plan to do a private Helix run to look at test failures? We were nearly clean on CLR based UWP runs. ProjectN ones had a bunch of brokenness, but it might be worth checking how much it got worse while we were dark.

nattress · 2018-03-01T20:48:47Z

Yes, Jose is helping me get one kicked off so we can see what state UAP tests are in. Once the CI is green I hope to merge this since branch churn is busting this a couple times a day this week.

jkotas · 2018-03-01T20:57:07Z

src/System.Net.NameResolution/src/System.Net.NameResolution.csproj

@@ -129,10 +131,10 @@
    <Compile Include="$(CommonPath)\Microsoft\Win32\SafeHandles\SafeLibraryHandle.cs">


Should this be under Condition="'$(TargetGroup)' != 'uap'" as well?

Yes, that doesn't break the build.

jkotas · 2018-03-01T20:59:39Z

src/System.Net.NameResolution/tests/PalTests/System.Net.NameResolution.Pal.Tests.csproj

      <Link>Common\System\Net\ByteOrder.cs</Link>
    </Compile>
  </ItemGroup>
  <ItemGroup Condition=" '$(TargetsWindows)' == 'true' ">
    <Compile Include="..\..\src\System\Net\NameResolutionPal.Windows.cs">
      <Link>ProductionCode\System\Net\NameResolutionPal.Windows.cs</Link>
    </Compile>
+    <Compile Include="..\..\src\System\Net\NameResolutionPal.Windows.Uap.cs" Condition="'$(TargetGroup)' == 'uap'">


Nit: I think this should be just NameResolutionPal.Uap.cs to follow the convention

With `Vector<T>` now residing in System.Private.CoreLib, directly reference System.Private.Corelib instead of contracts just like netcoreapp does. Add uapaot flavor since System.Private.CoreLib has a different strong name key on that platform.

Add the helper method `System.IO.PathInternal.EnsureExtendedPrefixOverMaxPath` to Common\src\System\IO\PathInternal.Windows.cs. It was previously not visible to UAP since Common\src\CoreLib\System\IO\PathInternal.Windows.cs is not included in UAP compilation.

SocketsHttpHandler is not supported in uap and is currently leaking into UAP builds since they've been disabled for a few weeks. Adjust the contract so it's not available to UAP, and clean the implementation assembly so SocketsHttpHandler internals don't leak out into HttpClientHandler.Core.cs.

`System.Net.NameResolutionPal.GetAddrInfoExSupportsOverlapped` uses LoadLibraryExW which is not compatible with UAP (which only supports LoadLibrary of DLLs within a container). Refactor the helper so UAP returns false.

This reverts commit 120dce4.

This reverts commit 2202b4f.

This reverts commit ef79caf.

- Refactor to remove code duplication in HttpClientHandler.Win32.cs - Suffix non-UAP source files with.Win32.cs instead of NetCoreApp.cs to follow branch convention - Fix spacing

- Baseline the ApiCompat and GenFacades failures in System.Runtime.Extensions and System.Threading.ThreadPool. - Adjust naming of files to match branch conventions

Fix up some configuration issues with uap packaging. The CoreFX.Private.TestUtilities packaging needs updated build tools which can map between uwp6.0 and netstandard2.0. #ifdef out Thread.GetCurrentProcessorId until we get a new System.Private.CoreLib.

- Fix a couple Nits - Fix recent commit breaking UAP

stephentoub · 2018-03-02T02:02:58Z

I have a change that was already checked into coreclr that alters some core methods. Does this change mean we now need to wait for a new net native build with those changes before we can update corefx? coreclr and corefx will be inconsistent until that's merged.

nattress · 2018-03-02T02:08:40Z

Changes to System.Private.CoreLib? You should be able to baseline / if-def around them (or just disable building the assembly for UAP if it's not easily scoped out) until the same change flows in from net native.

stephentoub · 2018-03-02T02:12:09Z

Changes to System.Private.CoreLib?

Yes, in particular to a signature of a virtual method on Stream.

jkotas · 2018-03-02T02:17:31Z

This change would be really ugly to ifdef. My recommendation would be to disable building of as many projects as necessary to get your change through.

joperezr · 2018-03-02T17:22:16Z

buildpipeline/pipelinejobs.groovy

@@ -27,8 +27,8 @@ def configurations = [
    ['TGroup':"netcoreapp", 'Pipeline':osxPipeline, 'Name':'OSX', 'ForPR':"Debug-x64", 'Arch':['x64']],
    ['TGroup':"netcoreapp", 'Pipeline':winPipeline, 'Name':'Windows' , 'ForPR':"Debug-x64|Release-x86"],
    ['TGroup':"netfx",      'Pipeline':winPipeline, 'Name':'NETFX', 'ForPR':"Release-x86"],
-//  ['TGroup':"uap",        'Pipeline':winPipeline, 'Name':'UWP CoreCLR', 'ForPR':"Debug-x64"],
-//  ['TGroup':"uapaot",     'Pipeline':winPipeline, 'Name':'UWP NETNative', 'ForPR':"Release-x86"],
+    ['TGroup':"uap",        'Pipeline':winPipeline, 'Name':'UWP CoreCLR', 'ForPR':"Debug-x64"],


We might want to consider not running tests on this leg either, since we know a few will be broken. If we don't do that, we'll have to basically ignore this leg if it fails on a PR.

Right, that's what I have done in #27640

joperezr · 2018-03-02T17:24:02Z

src/CoreFx.Private.TestUtilities/ref/CoreFx.Private.TestUtilities.csproj

@@ -6,7 +6,7 @@
    <ShouldWriteSigningRequired>false</ShouldWriteSigningRequired>
    <AllowReferenceFromRuntime>true</AllowReferenceFromRuntime>
    <RuntimeProjectFile>$(ProjectDir)\external\test-runtime\XUnit.Runtime.depproj</RuntimeProjectFile>
-    <PackageTargetFramework>netstandard2.0</PackageTargetFramework> 
+    <PackageTargetFramework>netstandard2.0;$(UAPvNextTFM)</PackageTargetFramework>


This should get undone before merging to release/2.1 since they are already using the new nuget mappings. I will try to fix this before the merge.

joperezr · 2018-03-02T17:28:24Z

src/System.Numerics.Vectors/ref/Configurations.props

@@ -9,6 +9,7 @@
    <BuildConfigurations>
      netcoreapp;
      uap;
+      uapaot;


This is wrong, why do we want a new configuration if uap and uapaot produce the exact same dll?

Yes, you are right.

Deleted in #27663

joperezr · 2018-03-02T17:30:19Z

src/System.Numerics.Vectors/src/System.Numerics.Vectors.csproj

@@ -7,7 +7,7 @@
    <DocumentationFile>$(OutputPath)$(MSBuildProjectName).xml</DocumentationFile>
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <IsTargetingNetFx Condition="'$(TargetGroup)'=='netfx' OR '$(TargetGroup)'=='net46'">true</IsTargetingNetFx>
-    <IsTargetingNetCoreApp Condition="'$(TargetGroup)'=='netcoreapp'">true</IsTargetingNetCoreApp>
+    <IsTargetingNetCoreApp Condition="'$(TargetGroup)'=='netcoreapp' OR '$(TargetGroup)'=='uap' OR '$(TargetGroup)'=='uapaot'">true</IsTargetingNetCoreApp>


This doesn't look right. We probably need to change property name.

We use netcoreapp to netcoreapp+uap+uapaot in many places. Yes, it is confusing, but one more place does not really make a difference.

joperezr · 2018-03-02T17:30:49Z

src/System.Numerics.Vectors/src/Configurations.props

@@ -7,6 +7,7 @@
      net46;
    </PackageConfigurations>
    <BuildConfigurations>
+      uapaot-Windows_NT;


This is wrong I believe, we don't need a uapaot config since the uap config seems to be equivalent.

It is not equivalent.

* Build uapaot System.Numerics.Vectors With `Vector<T>` now residing in System.Private.CoreLib, directly reference System.Private.Corelib instead of contracts just like netcoreapp does. Add uapaot flavor since System.Private.CoreLib has a different strong name key on that platform. * Make EnsureExtendedPrefixOverMaxPath available to UAP Add the helper method `System.IO.PathInternal.EnsureExtendedPrefixOverMaxPath` to Common\src\System\IO\PathInternal.Windows.cs. It was previously not visible to UAP since Common\src\CoreLib\System\IO\PathInternal.Windows.cs is not included in UAP compilation. * Keep SocketsHttpHandler out of UAP builds SocketsHttpHandler is not supported in uap and is currently leaking into UAP builds since they've been disabled for a few weeks. Adjust the contract so it's not available to UAP, and clean the implementation assembly so SocketsHttpHandler internals don't leak out into HttpClientHandler.Core.cs. * Fix GetAddrInfoExSupportsOverlapped UAP build break `System.Net.NameResolutionPal.GetAddrInfoExSupportsOverlapped` uses LoadLibraryExW which is not compatible with UAP (which only supports LoadLibrary of DLLs within a container). Refactor the helper so UAP returns false. * Revert "Disable UAP legs per dotnet/corefx#26802 (dotnet/corefx#26822)" This reverts commit dotnet/corefx@120dce4. * Revert "Disable UAP configurations in all configurations build" This reverts commit dotnet/corefx@2202b4f. * Revert "Disable UAP official builds (dotnet/corefx#26871)" This reverts commit dotnet/corefx@ef79caf. - Baseline the ApiCompat and GenFacades failures in System.Runtime.Extensions and System.Threading.ThreadPool. - Adjust naming of files to match branch conventions - Fix up some configuration issues with uap packaging. The CoreFX.Private.TestUtilities packaging needs updated build tools which can map between uwp6.0 and netstandard2.0. - ifdef out Thread.GetCurrentProcessorId until we get a new System.Private.CoreLib. Commit migrated from dotnet/corefx@c9cdfba

jkotas reviewed Feb 28, 2018

View reviewed changes

ahsonkhan reviewed Feb 28, 2018

View reviewed changes

ahsonkhan assigned nattress Feb 28, 2018

ahsonkhan added the area-Infrastructure-libraries label Feb 28, 2018

JeremyKuhne reviewed Feb 28, 2018

View reviewed changes

JeremyKuhne suggested changes Feb 28, 2018

View reviewed changes

MichalStrehovsky mentioned this pull request Feb 28, 2018

Merge master to nmirror dotnet/corert#5466

Closed

jkotas reviewed Feb 28, 2018

View reviewed changes

nattress force-pushed the fixuap branch from e139c5f to 14a4407 Compare February 28, 2018 20:18

jkotas reviewed Mar 1, 2018

View reviewed changes

joshfree approved these changes Mar 1, 2018

View reviewed changes

joshfree requested review from ericstj and joperezr March 1, 2018 00:55

nattress force-pushed the fixuap branch from d799751 to c279cae Compare March 1, 2018 02:22

JeffCyr reviewed Mar 1, 2018

View reviewed changes

nattress force-pushed the fixuap branch from e2045fd to b6601f2 Compare March 1, 2018 20:45

jkotas reviewed Mar 1, 2018

View reviewed changes

Build uapaot System.Numerics.Vectors

614f1ff

With `Vector<T>` now residing in System.Private.CoreLib, directly reference System.Private.Corelib instead of contracts just like netcoreapp does. Add uapaot flavor since System.Private.CoreLib has a different strong name key on that platform.

nattress added 11 commits March 1, 2018 13:30

Fix GetAddrInfoExSupportsOverlapped UAP build break

d8b26f8

`System.Net.NameResolutionPal.GetAddrInfoExSupportsOverlapped` uses LoadLibraryExW which is not compatible with UAP (which only supports LoadLibrary of DLLs within a container). Refactor the helper so UAP returns false.

Revert "Disable UAP legs per #26802 (dotnet#26822)"

da98236

This reverts commit 120dce4.

Revert "Disable UAP configurations in all configurations build"

04026cf

This reverts commit 2202b4f.

Revert "Disable UAP official builds (dotnet#26871)"

244c5f6

This reverts commit ef79caf.

Code Review Feedback

0b846b1

- Refactor to remove code duplication in HttpClientHandler.Win32.cs - Suffix non-UAP source files with.Win32.cs instead of NetCoreApp.cs to follow branch convention - Fix spacing

Code Review Feedback II

0701cea

- Baseline the ApiCompat and GenFacades failures in System.Runtime.Extensions and System.Threading.ThreadPool. - Adjust naming of files to match branch conventions

Missed a spot

1c35e7d

Fix up packaging build

01311ca

Fix up some configuration issues with uap packaging. The CoreFX.Private.TestUtilities packaging needs updated build tools which can map between uwp6.0 and netstandard2.0. #ifdef out Thread.GetCurrentProcessorId until we get a new System.Private.CoreLib.

Code Review Feedback III

68ba394

- Fix a couple Nits - Fix recent commit breaking UAP

nattress force-pushed the fixuap branch from b6601f2 to 68ba394 Compare March 1, 2018 22:03

jkotas approved these changes Mar 1, 2018

View reviewed changes

nattress merged commit c9cdfba into dotnet:master Mar 2, 2018

ahsonkhan mentioned this pull request Mar 2, 2018

Add GetPosition overload to ReadOnlySequence #27633

Merged

joperezr reviewed Mar 2, 2018

View reviewed changes

karelz added this to the 2.1.0 milestone Mar 10, 2018

MichalStrehovsky mentioned this pull request Oct 15, 2018

Build S.R.Metadata on AOT configs same as netcoreapp #32823

Merged

		@@ -132,7 +134,7 @@
		<Compile Include="$(CommonPath)\Interop\Windows\Kernel32\Interop.GetProcAddress.cs">

		@@ -129,10 +131,10 @@
		<Compile Include="$(CommonPath)\Microsoft\Win32\SafeHandles\SafeLibraryHandle.cs">

Re-enable UAP builds #27531

Re-enable UAP builds #27531

Conversation

nattress commented Feb 28, 2018 • edited Loading

nattress commented Feb 28, 2018

jkotas commented Feb 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyKuhne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Mar 1, 2018

nattress commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Mar 2, 2018

nattress commented Mar 2, 2018

stephentoub commented Mar 2, 2018

jkotas commented Mar 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nattress commented Feb 28, 2018 •

edited

Loading

jkotas Mar 1, 2018 •

edited

Loading