Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
28cd526
Update stage 0 CLI
dsplaisted Oct 18, 2017
13989f1
Fix SDK build
dsplaisted Oct 19, 2017
4e1877e
Include updated Http and Compression libraries for .NET 4.7.1 if nece…
dsplaisted Oct 19, 2017
222fc26
Fix test and update comments
dsplaisted Oct 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion DotnetCLIVersion.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
15.5.0-preview-007044
2.1.1-preview-007089
46 changes: 39 additions & 7 deletions src/Tasks/Microsoft.NET.Build.Extensions.Tasks/GetDependsOnNETStandard.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What value are these comments providing?

// .NET Standard facade version: 4.2.0.0
// Encountered conflict between 'Platform:System.Net.Http.dll' and 'CopyLocal:C:\git\dotnet-sdk\packages\system.net.http\4.3.0\runtimes\win\lib\net46\System.Net.Http.dll'. Choosing 'CopyLocal:C:\git\dotnet-sdk\packages\system.net.http\4.3.0\runtimes\win\lib\net46\System.Net.Http.dll' because AssemblyVersion '4.1.1.0' is greater than '4.0.0.0'.
// .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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


private const string SystemNetHttpAssemblyName = "System.Net.Http";
private static readonly Version SystemNetHttpMinVersion = new Version(4, 1, 0, 0);

/// <summary>
/// Set of reference items to analyze.
/// </summary>
Expand All @@ -34,13 +45,29 @@ public partial class GetDependsOnNETStandard : TaskBase
[Output]
public bool DependsOnNETStandard { get; set; }

/// <summary>
/// True if any of the references depend on a version of System.IO.Compression at least 4.0.1.0 (ie from a NuGet package)
/// </summary>
[Output]
public bool DependsOnNuGetCompression { get; set; }

/// <summary>
/// True if any of the references depend on a version of System.Net.Http at least 4.0.1.0 (ie from a NuGet package)
/// </summary>
[Output]
public bool DependsOnNuGetHttp { get; set; }

protected override void ExecuteCore()
{
DependsOnNETStandard = AnyReferenceDependsOnNETStandard();
ProcessReferences();
}

private bool AnyReferenceDependsOnNETStandard()
private void ProcessReferences()
{
DependsOnNETStandard = false;
DependsOnNuGetCompression = false;
DependsOnNuGetHttp = false;

foreach (var reference in References)
{
var referenceSourcePath = ItemUtilities.GetSourcePath(reference);
Expand All @@ -49,10 +76,15 @@ private bool AnyReferenceDependsOnNETStandard()
{
try
{
if (GetFileDependsOnNETStandard(referenceSourcePath))
{
return true;
}
bool dependsOnNETStandard;
bool dependsOnNuGetCompression;
bool dependsOnNuGetHttp;

GetFileDependsOn(referenceSourcePath, out dependsOnNETStandard, out dependsOnNuGetCompression, out dependsOnNuGetHttp);

DependsOnNETStandard |= dependsOnNETStandard;
DependsOnNuGetCompression |= dependsOnNuGetCompression;
DependsOnNuGetHttp |= dependsOnNuGetHttp;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
catch (Exception e) when (IsReferenceException(e))
{
Expand All @@ -62,7 +94,7 @@ private bool AnyReferenceDependsOnNETStandard()
}
}

return false;

}

// ported from MSBuild's ReferenceTable.SetPrimaryAssemblyReferenceItem
Expand Down
51 changes: 39 additions & 12 deletions src/Tasks/Microsoft.NET.Build.Extensions.Tasks/GetDependsOnNETStandard.net46.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,15 @@ public partial class GetDependsOnNETStandard
// - load / JIT cost of SRM and its closure
// - deal with bindingRedirects/unification needed to load SRM's closure.

internal static bool GetFileDependsOnNETStandard(string filePath)
internal static void GetFileDependsOn(string filePath,
out bool dependsOnNETStandard,
out bool dependsOnNuGetCompression,
out bool dependsOnNuGetHttp)
{
dependsOnNETStandard = false;
dependsOnNuGetCompression = false;
dependsOnNuGetHttp = false;

// Ported from Microsoft.Build.Tasks.AssemblyInformation
if (Environment.OSVersion.Platform == PlatformID.Unix || Environment.OSVersion.Platform == PlatformID.MacOSX)
{
Expand All @@ -34,7 +41,22 @@ internal static bool GetFileDependsOnNETStandard(string filePath)
{
if (referencedAssembly.Name.Equals(NetStandardAssemblyName, StringComparison.Ordinal))
{
return true;
dependsOnNETStandard = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More potential for short-circuiting here, and below.

}
else if (referencedAssembly.Name.Equals(SystemRuntimeAssemblyName, StringComparison.Ordinal) &&
referencedAssembly.Version >= SystemRuntimeMinVersion)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
dependsOnNETStandard = true;
}
else if (referencedAssembly.Name.Equals(SystemIOCompressionAssemblyName, StringComparison.Ordinal) &&
referencedAssembly.Version >= SystemIOCompressionMinVersion)
{
dependsOnNuGetCompression = true;
}
else if (referencedAssembly.Name.Equals(SystemNetHttpAssemblyName, StringComparison.Ordinal) &&
referencedAssembly.Version >= SystemNetHttpMinVersion)
{
dependsOnNuGetHttp = true;
}
}
}
Expand Down Expand Up @@ -105,20 +127,26 @@ internal static bool GetFileDependsOnNETStandard(string filePath)
out flags);

var assemblyName = assemblyNameBuffer.ToString();
var assemblyVersion = new Version(assemblyMD.usMajorVersion, assemblyMD.usMinorVersion, assemblyMD.usBuildNumber, assemblyMD.usRevisionNumber);

if (assemblyName.Equals(NetStandardAssemblyName, StringComparison.Ordinal))
{
return true;
dependsOnNETStandard = true;
}

if (assemblyName.Equals(SystemRuntimeAssemblyName, StringComparison.Ordinal))
else if (assemblyName.Equals(SystemRuntimeAssemblyName, StringComparison.Ordinal) &&
assemblyVersion >= SystemRuntimeMinVersion)
{
var assemblyVersion = new Version(assemblyMD.usMajorVersion, assemblyMD.usMinorVersion, assemblyMD.usBuildNumber, assemblyMD.usRevisionNumber);

if (assemblyVersion >= SystemRuntimeMinVersion)
{
return true;
}
dependsOnNETStandard = true;
}
else if (assemblyName.Equals(SystemIOCompressionAssemblyName, StringComparison.Ordinal) &&
assemblyVersion >= SystemIOCompressionMinVersion)
{
dependsOnNuGetCompression = true;
}
else if (assemblyName.Equals(SystemNetHttpAssemblyName, StringComparison.Ordinal) &&
assemblyVersion >= SystemNetHttpMinVersion)
{
dependsOnNuGetHttp = true;
}
}
} while (fetched > 0);
Expand All @@ -141,7 +169,6 @@ internal static bool GetFileDependsOnNETStandard(string filePath)
}
}
}
return false;
}

#region Interop
Expand Down
26 changes: 21 additions & 5 deletions src/Tasks/Microsoft.NET.Build.Extensions.Tasks/GetDependsOnNETStandard.netstandard.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,14 @@ namespace Microsoft.NET.Build.Tasks
{
public partial class GetDependsOnNETStandard
{
internal static bool GetFileDependsOnNETStandard(string filePath)
internal static void GetFileDependsOn(string filePath,
out bool dependsOnNETStandard,
out bool dependsOnNuGetCompression,
out bool dependsOnNuGetHttp)
{
dependsOnNETStandard = false;
dependsOnNuGetCompression = false;
dependsOnNuGetHttp = false;
using (var assemblyStream = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.Delete | FileShare.Read))
using (PEReader peReader = new PEReader(assemblyStream, PEStreamOptions.LeaveOpen))
{
Expand All @@ -27,20 +33,30 @@ internal static bool GetFileDependsOnNETStandard(string filePath)

if (reader.StringComparer.Equals(reference.Name, NetStandardAssemblyName))
{
return true;
dependsOnNETStandard = true;
}

if (reader.StringComparer.Equals(reference.Name, SystemRuntimeAssemblyName) &&
reference.Version >= SystemRuntimeMinVersion)
{
return true;
dependsOnNETStandard = true;
}

if (reader.StringComparer.Equals(reference.Name, SystemIOCompressionAssemblyName) &&
reference.Version >= SystemIOCompressionMinVersion)
{
dependsOnNuGetCompression = true;
}

if (reader.StringComparer.Equals(reference.Name, SystemNetHttpAssemblyName) &&
reference.Version >= SystemNetHttpMinVersion)
{
dependsOnNuGetHttp = true;
}
}
}
}
}

return false;
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Tasks/Microsoft.NET.Build.Extensions.Tasks/Microsoft.NET.Build.Extensions.Tasks.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@

<!-- Remove files from copy local that would not be published as they are provided by the platform package -->
<!-- https://github.com/dotnet/sdk/issues/933 tracks a first class feature for this -->
<Target Name="FilterCopyLocal" DependsOnTargets="RunResolvePublishAssemblies" BeforeTargets="ResolveLockFileCopyLocalProjectDeps">
<Target Name="FilterCopyLocal" DependsOnTargets="PrepareForBuild;RunResolvePublishAssemblies" BeforeTargets="ResolveLockFileCopyLocalProjectDeps">
<ItemGroup>
<_CopyLocalButNotPublished Include="@(AllCopyLocalItems)" Exclude="@(ResolvedAssembliesToPublish)" />
<AllCopyLocalItems Remove="@(_CopyLocalButNotPublished)" />
Expand Down
30 changes: 27 additions & 3 deletions ...rosoft/Microsoft.NET.Build.Extensions/Microsoft.NET.Build.Extensions.NETFramework.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,20 @@ Copyright (c) .NET Foundation. All rights reserved.
<PropertyGroup Condition="'$(DependsOnNETStandard)' == '' AND '$(NETStandardInbox)' != 'true'">
<DependsOnNETStandard Condition="('%(_ResolvedProjectReferencePaths.TargetFrameworkIdentifier)' == '.NETStandard') And ('%(_ResolvedProjectReferencePaths.TargetFrameworkVersion)' >= '1.5')">true</DependsOnNETStandard>
</PropertyGroup>

<PropertyGroup Condition="'@(_CandidateNETStandardReferences)' != ''">
<!-- Use an intermediate property to simplify condition. Run GetDependsOnNETStandard if either: -->
<!-- 1. The .NET Standard 2.0 support need to be injected -->
<_RunGetDependsOnNETStandard Condition="'$(DependsOnNETStandard)' == '' AND '$(NETStandardInbox)' != 'true'">true</_RunGetDependsOnNETStandard>
<!-- 2. The target framework is .NET 4.7.1, which needs to special case System.IO.Compression and System.Net.Http -->
<_RunGetDependsOnNETStandard Condition="'$(_TargetFrameworkVersionWithoutV)' == '4.7.1'">true</_RunGetDependsOnNETStandard>
</PropertyGroup>

<GetDependsOnNETStandard Condition="'$(DependsOnNETStandard)' == '' AND '$(NETStandardInbox)' != 'true' AND '@(_CandidateNETStandardReferences)' != ''"
<GetDependsOnNETStandard Condition="'$(_RunGetDependsOnNETStandard)' == 'true'"
References="@(_CandidateNETStandardReferences)">
<Output TaskParameter="DependsOnNETStandard" PropertyName="DependsOnNETStandard" />
<Output TaskParameter="DependsOnNuGetCompression" PropertyName="DependsOnNuGetCompression" />
<Output TaskParameter="DependsOnNuGetHttp" PropertyName="DependsOnNuGetHttp" />
</GetDependsOnNETStandard>

<!-- prevent using an older SDK version with NETStandard2.0 references -->
Expand All @@ -57,8 +67,20 @@ Copyright (c) .NET Foundation. All rights reserved.
<NETBuildExtensionsError Condition="'$(DependsOnNETStandard)' == 'true' AND '$(NETStandardInbox)' != 'true' AND '$(_UsingOldSDK)' == 'true'"
ResourceName="UnsupportedSDKVersionForNetStandard20"/>

<!--
.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.)
-->
<ItemGroup Condition="'$(_TargetFrameworkVersionWithoutV)' == '4.7.1'">
<_NETStandardLibraryNETFrameworkLib Include="$(MSBuildThisFileDirectory)\net461\lib\System.Net.Http.dll" Condition="'$(DependsOnNuGetHttp)' == 'true' Or '$(DependsOnNETStandard)' == 'true'"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<_NETStandardLibraryNETFrameworkLib Include="$(MSBuildThisFileDirectory)\net461\lib\System.IO.Compression.dll" Condition="'$(DependsOnNuGetCompression)' == 'true' Or '$(DependsOnNETStandard)' == 'true'"/>
</ItemGroup>

<!-- if any reference depends on netstandard and it is not inbox, add references and implementation assemblies for netstandard2.0 -->
<ItemGroup Condition="'$(DependsOnNETStandard)' == 'true' AND '$(NETStandardInbox)' != 'true'">
<ItemGroup Condition="'$(DependsOnNETStandard)' == 'true' AND '$(NETStandardInbox)' != 'true' AND '$(_TargetFrameworkVersionWithoutV)' != '4.7.1'">
<_NETStandardLibraryNETFrameworkLib Condition="'$(_TargetFrameworkVersionWithoutV)' &gt;= '4.7'"
Include="$(MSBuildThisFileDirectory)net47\lib\*.dll" />
<_NETStandardLibraryNETFrameworkLib Condition="'$(_TargetFrameworkVersionWithoutV)' &gt;= '4.6.2'"
Expand All @@ -67,7 +89,9 @@ Copyright (c) .NET Foundation. All rights reserved.
<_NETStandardLibraryNETFrameworkLib Condition="'$(_TargetFrameworkVersionWithoutV)' &gt;= '4.6.1'"
Include="$(MSBuildThisFileDirectory)net461\lib\*.dll"
Exclude="@(_NETStandardLibraryNETFrameworkLib->'$(MSBuildThisFileDirectory)net461\lib\%(FileName).dll')" />

</ItemGroup>

<ItemGroup Condition="'@(_NETStandardLibraryNETFrameworkLib)' != ''">
<!-- Remove simple name references if we're directly providing a reference assembly to the compiler. For example,
consider a project with a Reference Include="System.Net.Http" or "System.IO.Compression", which are both in
_NETStandardLibraryNETFrameworkReference.
Expand Down
2 changes: 1 addition & 1 deletion src/Tasks/Microsoft.NET.Build.Tasks/Microsoft.NET.Build.Tasks.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@

<!-- Remove files from copy local that would not be published as they are provided by the platform package -->
<!-- https://github.com/dotnet/sdk/issues/933 tracks a first class feature for this -->
<Target Name="FilterCopyLocal" DependsOnTargets="RunResolvePublishAssemblies" BeforeTargets="ResolveLockFileCopyLocalProjectDeps">
<Target Name="FilterCopyLocal" DependsOnTargets="PrepareForBuild;RunResolvePublishAssemblies" BeforeTargets="ResolveLockFileCopyLocalProjectDeps">
<ItemGroup>
<_CopyLocalButNotPublished Include="@(AllCopyLocalItems)" Exclude="@(ResolvedAssembliesToPublish)" />
<AllCopyLocalItems Remove="@(_CopyLocalButNotPublished)" />
Expand Down
Loading