Include updated Http and Compression libraries for .NET 4.7.1 if necessary #1667

dsplaisted · 2017-10-19T06:39:39Z

Fixes #1647. .NET 4.7.1 has support for .NET Standard 2.0 built-in, so most of the facades aren't necessary. However, the assembly versions of System.Net.Http and System.IO.Compression in .NET 4.7.1 are still 4.0.0.0. This means that the versions from the contract NuGet packages (which are versioned 4.1.0.0 or higher) would be preferred to the in-box version (which is newer). So if there is a dependency on the "contract" version of these DLLs, or on netstandard.dll, we use the 4.2.0.0 version of the DLLs from the .NET Standard 2.0 "facades". (Though these DLLs are not actually facades, they contain the implementation.)

Also updates the stage 0 version of the CLI, and fixes a failure it caused with our FilterCopyLocal targets, as described here.

With dotnet#1618, conflict resolution now depends on TargetFrameworkDirectory property, and the GetReferenceAssemblyPaths target to set it. However, the custom FilterCopyLocal target in our projects was causing GetReferenceAssemblyPaths to run before GetFrameworkPaths, and when GetFrameworkPaths was subsequently run, it reset the TargetFrameworkDirectory property to v4.0.30319. This change adds a dependency to PrepareForBuild so that both targets run in the right order.

…ssary

dsplaisted · 2017-10-19T18:49:33Z

@ericstj and @nguerrera for review. Please give this a good look as it would have been easy to make a mistake with these changes (though I think I've got them right and the tests help give me confidence).

ericstj · 2017-10-20T19:31:45Z

src/Tasks/Microsoft.NET.Build.Extensions.Tasks/GetDependsOnNETStandard.cs

+
+                        DependsOnNETStandard |= dependsOnNETStandard;
+                        DependsOnNuGetCompression |= dependsOnNuGetCompression;
+                        DependsOnNuGetHttp |= dependsOnNuGetHttp;


Consider a short-circuit path once everything is set to true.

ericstj · 2017-10-20T19:33:03Z

src/Tasks/Microsoft.NET.Build.Extensions.Tasks/GetDependsOnNETStandard.cs

+        //  .NET Standard facade version: 4.2.0.0
+
+        private const string SystemIOCompressionAssemblyName = "System.IO.Compression";
+        private static readonly Version SystemIOCompressionMinVersion = new Version(4, 1, 0, 0);


Here you use 4.1.0.0, but below you say 4.0.1.0. Which is it?

ericstj · 2017-10-20T19:34:21Z

@alexghi @weshaggard PTAL as well.

ericstj · 2017-10-20T19:39:24Z

src/Tasks/Microsoft.NET.Build.Extensions.Tasks/GetDependsOnNETStandard.cs

@@ -22,6 +22,17 @@ public partial class GetDependsOnNETStandard : TaskBase
        private const string SystemRuntimeAssemblyName = "System.Runtime";
        private static readonly Version SystemRuntimeMinVersion = new Version(4, 1, 0, 0);

+        //   Encountered conflict between 'Platform:System.IO.Compression.dll' and 'CopyLocal:C:\git\dotnet-sdk\packages\system.io.compression\4.3.0\runtimes\win\lib\net46\System.IO.Compression.dll'.  Choosing 'CopyLocal:C:\git\dotnet-sdk\packages\system.io.compression\4.3.0\runtimes\win\lib\net46\System.IO.Compression.dll' because AssemblyVersion '4.1.2.0' is greater than '4.0.0.0'.


What value are these comments providing?

ericstj · 2017-10-20T19:43:25Z

src/Tasks/Microsoft.NET.Build.Extensions.Tasks/GetDependsOnNETStandard.net46.cs

@@ -34,7 +41,22 @@ internal static bool GetFileDependsOnNETStandard(string filePath)
                {
                    if (referencedAssembly.Name.Equals(NetStandardAssemblyName, StringComparison.Ordinal))
                    {
-                        return true;
+                        dependsOnNETStandard = true;


More potential for short-circuiting here, and below.

weshaggard · 2017-10-20T19:44:15Z

...Microsoft/Microsoft.NET.Build.Extensions/Microsoft.NET.Build.Extensions.NETFramework.targets

+      contain the implementation.)
+      -->
+    <ItemGroup Condition="'$(_TargetFrameworkVersionWithoutV)' == '4.7.1'">
+      <_NETStandardLibraryNETFrameworkLib Include="$(MSBuildThisFileDirectory)\net461\lib\System.Net.Http.dll" Condition="'$(DependsOnNuGetHttp)' == 'true' Or '$(DependsOnNETStandard)' == 'true'"/>


Is it worth the extra complexity to try and detect references to these? Why not just always include them in 4.7.1 like we do for the earlier versions?

+1. I don't like that we're adding all this code to workaround a framework bug.

What is the case where DependsOnNETStandard==false (which is actually system.runtime >= some-ver or netstandard) but DependsOnNugetXxx==true. Which test covers that?

@weshaggard @nguerrera

Is it worth the extra complexity to try and detect references to these? Why not just always include them in 4.7.1 like we do for the earlier versions?

Should we include these libraries in .NET 4.7.1 apps that don't reference .NET Standard assemblies at all? That's what this prevents. We can't just use the existing logic to detect usage of .NET Standard >= 1.5, because we need the updated libraries if they are used by a .NET Standard 1.4 or lower library too.

I think we should use whatever logic we are using to detect netstandard for both 2.0 and for earlier.

@alexghi will give you more data but we believe this set might be larger now.

nguerrera · 2017-10-20T20:00:36Z

Aside, but important: The change that broke our build (worked around in 13989f1) might also break IIS debugging in VS, which also has some custom targets that look at resolved publish assemblies.

EDIT: This is not an issue. I just tested with latest VS build having the Extensions changes in question and IIS debugging is working fine.

nguerrera · 2017-10-20T20:28:29Z

src/Tasks/Microsoft.NET.Build.Extensions.Tasks/GetDependsOnNETStandard.net46.cs

+                        dependsOnNETStandard = true;
+                    }
+                    else if (referencedAssembly.Name.Equals(SystemRuntimeAssemblyName, StringComparison.Ordinal) &&
+                             referencedAssembly.Version >= SystemRuntimeMinVersion)


Why didn't this code path check systemRuntimeMinVersion already? Bug?

nguerrera

There's offline discussion about the direction here.

dsplaisted · 2017-11-08T22:35:51Z

Closing this in favor of #1712

…719.9 (#1667) [main] Update dependencies from dotnet/arcade

dsplaisted added 2 commits October 18, 2017 15:10

Update stage 0 CLI

28cd526

dnfclas added the cla-already-signed label Oct 19, 2017

Include updated Http and Compression libraries for .NET 4.7.1 if nece…

4e1877e

…ssary

dsplaisted force-pushed the 1647-net471-facades branch from 8b8c387 to 4e1877e Compare October 19, 2017 18:42

dsplaisted changed the title ~~WIP: Include updated Http and Compression libraries for .NET 4.7.1 if necessary~~ Include updated Http and Compression libraries for .NET 4.7.1 if necessary Oct 19, 2017

Fix test and update comments

222fc26

ericstj reviewed Oct 20, 2017

View reviewed changes

weshaggard reviewed Oct 20, 2017

View reviewed changes

nguerrera reviewed Oct 20, 2017

View reviewed changes

nguerrera suggested changes Oct 30, 2017

View reviewed changes

jongalloway removed the cla-already-signed label Oct 31, 2017

dsplaisted closed this Nov 8, 2017

JL03-Yue pushed a commit that referenced this pull request Mar 19, 2024

Update dependencies from https://github.com/dotnet/arcade build 20220…

7481f5d

…719.9 (#1667) [main] Update dependencies from dotnet/arcade

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include updated Http and Compression libraries for .NET 4.7.1 if necessary #1667

Include updated Http and Compression libraries for .NET 4.7.1 if necessary #1667

dsplaisted commented Oct 19, 2017 •

edited

Loading

dsplaisted commented Oct 19, 2017

ericstj Oct 20, 2017

ericstj Oct 20, 2017

ericstj commented Oct 20, 2017

ericstj Oct 20, 2017

ericstj Oct 20, 2017

weshaggard Oct 20, 2017

nguerrera Oct 20, 2017

nguerrera Oct 20, 2017

dsplaisted Oct 20, 2017

weshaggard Oct 20, 2017

nguerrera commented Oct 20, 2017 •

edited

Loading

nguerrera Oct 20, 2017

nguerrera left a comment

dsplaisted commented Nov 8, 2017

Include updated Http and Compression libraries for .NET 4.7.1 if necessary #1667

Include updated Http and Compression libraries for .NET 4.7.1 if necessary #1667

Conversation

dsplaisted commented Oct 19, 2017 • edited Loading

dsplaisted commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstj commented Oct 20, 2017

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

nguerrera commented Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

nguerrera left a comment

Choose a reason for hiding this comment

dsplaisted commented Nov 8, 2017

dsplaisted commented Oct 19, 2017 •

edited

Loading

nguerrera commented Oct 20, 2017 •

edited

Loading