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

Corresponding PublishAOT changes to match the runtime cleanup changes #27159

Merged
merged 5 commits into from
Aug 15, 2022
Merged

Corresponding PublishAOT changes to match the runtime cleanup changes #27159

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d14b3d5
Corresponding PublishAOT changes to match the runtime cleanup changes
LakshanF Aug 14, 2022
a182e3b
FB
LakshanF Aug 15, 2022
0b58985
Restricting a low pri scenario validatin to windows ownly
LakshanF Aug 15, 2022
6acfc26
Update src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs
LakshanF Aug 15, 2022
373013d
naming fb
LakshanF Aug 15, 2022
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
29 changes: 27 additions & 2 deletions src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,10 @@ public class ProcessFrameworkReferences : TaskBase
public ITaskItem[] Crossgen2Packs { get; set; }

[Output]
public ITaskItem[] ILCompilerPacks { get; set; }
public ITaskItem[] HostILCompilerPacks { get; set; }

[Output]
public ITaskItem[] TargetILCompilerPacks { get; set; }

// Runtime packs which aren't available for the specified RuntimeIdentifier
[Output]
Expand Down Expand Up @@ -617,7 +620,29 @@ private bool AddAotOrR2RRuntimePackage(AotPackageType packageType, Version norma
}
else
{
ILCompilerPacks = new[] { newItem };
HostILCompilerPacks = new[] { newItem };
// ILCompiler supports cross target compilation. If there is a cross-target request, we need to download that package as well
// We expect RuntimeIdentifier to be defined during publish but can allow during build
if (RuntimeIdentifier != null)
{
var targetsRuntimeIdentifier = NuGetUtils.GetBestMatchingRid(runtimeGraph, RuntimeIdentifier, packSupportedRuntimeIdentifiers, out bool wasInGraph2);
if (targetsRuntimeIdentifier == null)
{
return false;
}
if (!hostRuntimeIdentifier.Equals(targetsRuntimeIdentifier))
LakshanF marked this conversation as resolved.
Show resolved Hide resolved
{
var runtimeIlcPackName = packPattern.Replace("**RID**", targetsRuntimeIdentifier);
TaskItem runtime2PackToDownload = new TaskItem(runtimeIlcPackName);
LakshanF marked this conversation as resolved.
Show resolved Hide resolved
runtime2PackToDownload.SetMetadata(MetadataKeys.Version, packVersion);
packagesToDownload.Add(runtime2PackToDownload);

var newItem2 = new TaskItem(runtimeIlcPackName);
newItem2.SetMetadata(MetadataKeys.NuGetPackageId, runtimeIlcPackName);
newItem2.SetMetadata(MetadataKeys.NuGetPackageVersion, packVersion);
TargetILCompilerPacks = new[] { newItem2 };
}
}
}

return true;
Expand Down
14 changes: 11 additions & 3 deletions .../Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,8 @@ Copyright (c) .NET Foundation. All rights reserved.
<Output TaskParameter="TargetingPacks" ItemName="TargetingPack" />
<Output TaskParameter="RuntimePacks" ItemName="RuntimePack" />
<Output TaskParameter="Crossgen2Packs" ItemName="Crossgen2Pack" />
<Output TaskParameter="ILCompilerPacks" ItemName="ILCompilerPack" />
<Output TaskParameter="HostILCompilerPacks" ItemName="HostILCompilerPack" />
<Output TaskParameter="TargetILCompilerPacks" ItemName="TargetILCompilerPack" />
<Output TaskParameter="UnavailableRuntimePacks" ItemName="UnavailableRuntimePack" />

</ProcessFrameworkReferences>
Expand Down Expand Up @@ -252,12 +253,19 @@ Copyright (c) .NET Foundation. All rights reserved.
</GetPackageDirectory>

<GetPackageDirectory
Items="@(ILCompilerPack)"
Items="@(HostILCompilerPack)"
PackageFolders="@(AssetsFilePackageFolder)">

<Output TaskParameter="Output" ItemName="ResolvedILCompilerPack" />
</GetPackageDirectory>


<GetPackageDirectory
Items="@(TargetILCompilerPack)"
PackageFolders="@(AssetsFilePackageFolder)">

<Output TaskParameter="Output" ItemName="ResolvedTargetILCompilerPack" />
</GetPackageDirectory>

<GetPackageDirectory
Items="@(PackAsToolShimAppHostPack)"
PackageFolders="@(AssetsFilePackageFolder)">
Expand Down
132 changes: 131 additions & 1 deletion src/Tests/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishAnAotApp.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,41 @@ public void NativeAot_hw_runs_with_no_warnings_when_PublishAot_is_enabled(string
}
}

[RequiresMSBuildVersionTheory("17.0.0.32901")]
[InlineData(ToolsetInfo.CurrentTargetFramework)]
public void NativeAot_hw_runs_with_no_warnings_when_PublishAot_is_false(string targetFramework)
{
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) || RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
{
var projectName = "HellowWorldNativeAotApp";
var rid = EnvironmentInfo.GetCompatibleRid(targetFramework);

var testProject = CreateHelloWorldTestProject(targetFramework, projectName, true);
testProject.AdditionalProperties["PublishAot"] = "false";
var testAsset = _testAssetsManager.CreateTestProject(testProject);

var publishCommand = new PublishCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
publishCommand
.Execute($"/p:RuntimeIdentifier={rid}")
.Should().Pass()
.And.NotHaveStdOutContaining("IL2026")
.And.NotHaveStdErrContaining("NETSDK1179")
.And.NotHaveStdErrContaining("warning");

var publishDirectory = publishCommand.GetOutputDirectory(targetFramework: targetFramework, runtimeIdentifier: rid).FullName;
var sharedLibSuffix = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? ".dll" : ".so";
var publishedDll = Path.Combine(publishDirectory, $"{projectName}{sharedLibSuffix}");
var publishedExe = Path.Combine(publishDirectory, $"{testProject.Name}{Constants.ExeSuffix}");

// PublishAot=false will be a normal publish
File.Exists(publishedDll).Should().BeTrue();

var command = new RunExeCommand(Log, publishedExe)
.Execute().Should().Pass()
.And.HaveStdOutContaining("Hello World");
}
}

[RequiresMSBuildVersionTheory("17.0.0.32901")]
[InlineData(ToolsetInfo.CurrentTargetFramework)]
public void NativeAot_app_runs_in_debug_with_no_config_when_PublishAot_is_enabled(string targetFramework)
Expand Down Expand Up @@ -245,7 +280,6 @@ public void NativeAot_hw_runs_with_PackageReference_PublishAot_is_enabled(string
if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
{
testProject.AdditionalProperties["StripSymbols"] = "true";
testProject.AdditionalProperties["ObjCopyName"] = "objcopy";
}
var testAsset = _testAssetsManager.CreateTestProject(testProject);

Expand Down Expand Up @@ -274,6 +308,102 @@ public void NativeAot_hw_runs_with_PackageReference_PublishAot_is_enabled(string
}
}

[RequiresMSBuildVersionTheory("17.0.0.32901")]
[InlineData(ToolsetInfo.CurrentTargetFramework)]
public void NativeAot_hw_runs_with_PackageReference_PublishAot_is_empty(string targetFramework)
{
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) || RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
{
var projectName = "HellowWorldNativeAotApp";
var rid = EnvironmentInfo.GetCompatibleRid(targetFramework);

var testProject = CreateHelloWorldTestProject(targetFramework, projectName, true);

// This will add a reference to a package that will also be automatically imported by the SDK
testProject.PackageReferences.Add(new TestPackageReference("Microsoft.DotNet.ILCompiler", "7.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.

Did you have a chance to think about how we could remove this source of non-determinism (#27098 (comment))?

I see two options:

  • Make it so that we know the version here. Pass it from the build somehow.
  • Make it so that we know the version at test build time. AFAIK this gets spilled into a csproj, so the version string could also be $(TheILCompilerPackageVersion) and it will expand then at the time the test project is built. We just need something in the SDK to tell us what ILCompiler package it's including.

Copy link
Member

Choose a reason for hiding this comment

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

(Maybe this doesn't have to be addressed here, just asking to make sure you saw it)


// Linux symbol files are embedded and require additional steps to be stripped to a separate file
// assumes /bin (or /usr/bin) are in the PATH
if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
{
testProject.AdditionalProperties["StripSymbols"] = "true";
}
var testAsset = _testAssetsManager.CreateTestProject(testProject);

var publishCommand = new PublishCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
publishCommand
.Execute($"/p:RuntimeIdentifier={rid}")
.Should().Pass();

var publishDirectory = publishCommand.GetOutputDirectory(targetFramework: targetFramework, runtimeIdentifier: rid).FullName;
var sharedLibSuffix = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? ".dll" : ".so";
var publishedDll = Path.Combine(publishDirectory, $"{projectName}{sharedLibSuffix}");
var publishedExe = Path.Combine(publishDirectory, $"{testProject.Name}{Constants.ExeSuffix}");
var symbolSuffix = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? ".pdb" : ".dbg";
var publishedDebugFile = Path.Combine(publishDirectory, $"{testProject.Name}{symbolSuffix}");

// NativeAOT published dir should not contain a non-host stand alone package
File.Exists(publishedDll).Should().BeFalse();
// The exe exist and should be native
File.Exists(publishedExe).Should().BeTrue();
File.Exists(publishedDebugFile).Should().BeTrue();
IsNativeImage(publishedExe).Should().BeTrue();

var command = new RunExeCommand(Log, publishedExe)
.Execute().Should().Pass()
.And.HaveStdOutContaining("Hello World");
}
}

[RequiresMSBuildVersionTheory("17.0.0.32901")]
[InlineData(ToolsetInfo.CurrentTargetFramework)]
public void NativeAot_hw_runs_with_cross_PackageReference_PublishAot_is_enabled(string targetFramework)
{
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && (RuntimeInformation.OSArchitecture == System.Runtime.InteropServices.Architecture.X64))
{
var projectName = "HellowWorldNativeAotApp";
var rid = "win-arm64";

var testProject = CreateHelloWorldTestProject(targetFramework, projectName, true);
testProject.AdditionalProperties["PublishAot"] = "true";

// This will add a reference to a package that will also be automatically imported by the SDK
testProject.PackageReferences.Add(new TestPackageReference("Microsoft.DotNet.ILCompiler", "7.0.0-*"));
testProject.PackageReferences.Add(new TestPackageReference("runtime.win-x64.Microsoft.DotNet.ILCompiler", "7.0.0-*"));
Comment on lines +369 to +371
Copy link
Member

Choose a reason for hiding this comment

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

I would expect crossbuild with PublishAot not to require an ILCompiler PackageReference.

People won't know what package version to include. The SDK should include all the necessary packages for them. The experience of managing ILCompiler packages version is subpar - we only want it for hotfixes, not as a way for people to consume PublishAot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. This requires the runtime changes to be present to set the runtime package directory to get the platform link,


var testAsset = _testAssetsManager.CreateTestProject(testProject);

var publishCommand = new PublishCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
publishCommand
.Execute($"/p:RuntimeIdentifier={rid}")
.Should().Pass();
}
}

[RequiresMSBuildVersionTheory("17.0.0.32901")]
[InlineData(ToolsetInfo.CurrentTargetFramework)]
public void NativeAot_hw_runs_with_cross_PackageReference_PublishAot_is_empty(string targetFramework)
{
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && (RuntimeInformation.OSArchitecture == System.Runtime.InteropServices.Architecture.X64))
{
var projectName = "HellowWorldNativeAotApp";
var rid = "win-arm64";

var testProject = CreateHelloWorldTestProject(targetFramework, projectName, true);

// This will add a reference to a package that will also be automatically imported by the SDK
testProject.PackageReferences.Add(new TestPackageReference("Microsoft.DotNet.ILCompiler", "7.0.0-*"));
testProject.PackageReferences.Add(new TestPackageReference("runtime.win-x64.Microsoft.DotNet.ILCompiler", "7.0.0-*"));

var testAsset = _testAssetsManager.CreateTestProject(testProject);

var publishCommand = new PublishCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
publishCommand
.Execute($"/p:RuntimeIdentifier={rid}")
.Should().Pass();
}
}

[RequiresMSBuildVersionTheory("17.0.0.32901")]
[InlineData(ToolsetInfo.CurrentTargetFramework)]
public void Only_Aot_warnings_are_produced_if_EnableAotAnalyzer_is_set(string targetFramework)
Expand Down