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.

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

Fix regression in selecting default RuntimeIdentifier #3543

Merged
merged 3 commits into from
Aug 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using Microsoft.Build.Framework;

namespace Microsoft.NET.Build.Tasks
{
public class GetDefaultPlatformTargetForNetFramework : TaskBase
{
public ITaskItem[] PackageDependencies { get; set; }

public ITaskItem[] NativeCopyLocalItems { get; set; }

[Output]
public string DefaultPlatformTarget { get; private set; }

private const string X86 = "x86";
private const string AnyCPU = "AnyCPU";

protected override void ExecuteCore()
{
// For .NET Framework projects, the SDK will select a default RuntimeIdentifier and PlatformTarget. If no
// native assets are found from NuGet packages, then the PlatformTarget will be reset to AnyCPU. See the
// comments in Microsoft.NET.RuntimeIdentifierInference.targets for details.
//
// Prior to the .NET Core 3.0 SDK, .NET Framework projects would only have a RuntimeIdentifier graph if the
// Microsoft.NETCore.Platforms package was (transitively) referenced. This meant that native assets would
// only be selected if the platforms package was referenced or if the RuntimeIdentifier matched exactly.
//
// Now that the RuntimeIdentifier graph is provided in the SDK, the logic in this task preserves the PlatformTarget
// behavior from earlier SDKs, even though with the RuntimeIdentifier graph supplied, there may be native
// assets selected where in prior SDKs there would not have been.

if (NativeCopyLocalItems == null || NativeCopyLocalItems.Length == 0)
{
DefaultPlatformTarget = AnyCPU;
return;
}

foreach (var packageDependency in PackageDependencies ?? Enumerable.Empty<ITaskItem>())
{
// If the Platforms package is in the dependencies, then any native assets imply an X86 default PlatformTarget
if (packageDependency.ItemSpec.Equals("Microsoft.NETCore.Platforms", StringComparison.OrdinalIgnoreCase))
{
DefaultPlatformTarget = X86;
return;
}
}

foreach (var nativeItem in NativeCopyLocalItems)
{
// If the Platforms package was not referenced, but there are native assets for the exact RID win7-x86,
// then the default PlatformTarget should be x86.
string pathInPackage = nativeItem.GetMetadata(MetadataKeys.PathInPackage);
if (pathInPackage.StartsWith("runtimes/win7-x86/", StringComparison.OrdinalIgnoreCase))
{
DefaultPlatformTarget = X86;
return;
}
}

// Otherwise, there would have been no native assets selected on pre-3.0 SDKs, so use AnyCPU as the
// default PlatformTarget
DefaultPlatformTarget = AnyCPU;
}
}
}
20 changes: 18 additions & 2 deletions src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,9 @@ public sealed class ResolvePackageAssets : TaskBase
[Output]
public ITaskItem[] ApphostsForShimRuntimeIdentifiers { get; private set; }

[Output]
public ITaskItem[] PackageDependencies { get; private set; }

/// <summary>
/// Messages from the assets file.
/// These are logged directly and therefore not returned to the targets (note private here).
Expand Down Expand Up @@ -275,7 +278,7 @@ public sealed class ResolvePackageAssets : TaskBase
////////////////////////////////////////////////////////////////////////////////////////////////////

private const int CacheFormatSignature = ('P' << 0) | ('K' << 8) | ('G' << 16) | ('A' << 24);
private const int CacheFormatVersion = 9;
private const int CacheFormatVersion = 10;
private static readonly Encoding TextEncoding = Encoding.UTF8;
private const int SettingsHashLength = 256 / 8;
private HashAlgorithm CreateSettingsHash() => SHA256.Create();
Expand Down Expand Up @@ -305,6 +308,7 @@ private void ReadItemGroups()
FrameworkAssemblies = reader.ReadItemGroup();
FrameworkReferences = reader.ReadItemGroup();
NativeLibraries = reader.ReadItemGroup();
PackageDependencies = reader.ReadItemGroup();
Copy link
Contributor

Choose a reason for hiding this comment

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

Bump format version

Copy link
Member Author

Choose a reason for hiding this comment

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

@nguerrera I've bumped it, can you re-approve?

PackageFolders = reader.ReadItemGroup();
ResourceAssemblies = reader.ReadItemGroup();
RuntimeAssemblies = reader.ReadItemGroup();
Expand Down Expand Up @@ -768,7 +772,8 @@ private void WriteItemGroups()
WriteItemGroup(WriteFrameworkAssemblies);
WriteItemGroup(WriteFrameworkReferences);
WriteItemGroup(WriteNativeLibraries);
WriteItemGroup(WritePackageFolders);
WriteItemGroup(WritePackageDependencies);
WriteItemGroup(WritePackageFolders);
WriteItemGroup(WriteResourceAssemblies);
WriteItemGroup(WriteRuntimeAssemblies);
WriteItemGroup(WriteRuntimeTargets);
Expand Down Expand Up @@ -1091,6 +1096,17 @@ private void WritePackageFolders()
}
}

private void WritePackageDependencies()
{
foreach (var library in _runtimeTarget.Libraries)
{
if (library.IsPackage())
{
WriteItem(library.Name);
}
}
}

private void WriteResourceAssemblies()
{
WriteItems(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,10 @@ Copyright (c) .NET Foundation. All rights reserved.
<OutputPath>$(OutputPath)$(RuntimeIdentifier)\</OutputPath>
</PropertyGroup>

<UsingTask TaskName="Microsoft.NET.Build.Tasks.GetDefaultPlatformTargetForNetFramework"
AssemblyFile="$(MicrosoftNETBuildTasksAssembly)" />


<!--
Switch our default .NETFramework CPU architecture choice back to AnyCPU before
compiling the exe if no copy-local native dependencies were resolved from NuGet
Expand All @@ -182,11 +186,14 @@ Copyright (c) .NET Foundation. All rights reserved.
AfterTargets="ResolvePackageAssets"
BeforeTargets="CoreCompile"
Condition="'$(_UsingDefaultPlatformTarget)' == 'true' and
'$(_UsingDefaultRuntimeIdentifier)' == 'true' and
'@(NativeCopyLocalItems)' == ''">
<PropertyGroup>
<PlatformTarget>AnyCPU</PlatformTarget>
</PropertyGroup>
'$(_UsingDefaultRuntimeIdentifier)' == 'true'">

<GetDefaultPlatformTargetForNetFramework PackageDependencies="@(PackageDependencies)"
NativeCopyLocalItems="@(NativeCopyLocalItems)">

<Output TaskParameter="DefaultPlatformTarget" PropertyName="PlatformTarget" />

</GetDefaultPlatformTargetForNetFramework>
</Target>

</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<Output TaskParameter="CompileTimeAssemblies" ItemName="ResolvedCompileFileDefinitions" />
<Output TaskParameter="TransitiveProjectReferences" ItemName="_TransitiveProjectReferences" />
<Output TaskParameter="PackageFolders" ItemName="AssetsFilePackageFolder" />
<Output TaskParameter="PackageDependencies" ItemName="PackageDependencies" />
</ResolvePackageAssets>

<ItemGroup Condition="'$(UseAppHostFromAssetsFile)' == 'true'">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,48 @@ public void It_builds_a_simple_desktop_app()
});
}

// Windows only because default RuntimeIdentifier only applies when current OS is Windows
[WindowsOnlyTheory]
[InlineData("Microsoft.DiasymReader.Native/1.7.0", false, "AnyCPU")]
[InlineData("Microsoft.DiasymReader.Native/1.7.0", true, "x86")]
[InlineData("SQLite/3.13.0", false, "x86")]
[InlineData("SQLite/3.13.0", true, "x86")]

public void PlatformTargetInferredCorrectly(string packageToReference, bool referencePlatformPackage, string expectedPlatform)
{
var testProject = new TestProject()
{
Name = "AutoRuntimeIdentifierTest",
TargetFrameworks = "net472",
IsSdkProject = true,
IsExe = true
};

var packageElements = packageToReference.Split('/');
string packageName = packageElements[0];
string packageVersion = packageElements[1];

testProject.PackageReferences.Add(new TestPackageReference(packageName, packageVersion));
if (referencePlatformPackage)
{
testProject.PackageReferences.Add(new TestPackageReference("Microsoft.NETCore.Platforms", "2.1.0"));
}

var testAsset = _testAssetsManager.CreateTestProject(testProject, identifier: packageName + "_" + referencePlatformPackage.ToString())
.Restore(Log, testProject.Name);

var buildCommand = new BuildCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));

buildCommand.Execute()
.Should()
.Pass();

var getValueCommand = new GetValuesCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name), testProject.TargetFrameworks, "PlatformTarget");

getValueCommand.Execute().Should().Should();
getValueCommand.GetValues().Single().Should().Be(expectedPlatform);
}

[WindowsOnlyTheory]
// If we don't set platformTarget and don't use native dependency, we get working AnyCPU app.
[InlineData("defaults", null, false, "Native code was not used (MSIL)")]
Expand Down