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

Jump to bottom

Prevent built-in artifacts output functionality from conflicting with Microsoft.Build.Artifacts SDK #33470

Merged
merged 3 commits into from
Jun 21, 2023
Merged

Prevent built-in artifacts output functionality from conflicting with Microsoft.Build.Artifacts SDK #33470

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
44640bc
Prevent built-in artifacts output functionality from conflicting with…
dsplaisted Jun 21, 2023
c4e6748
Fix test
dsplaisted Jun 21, 2023
00c594b
Fix test again
dsplaisted Jun 21, 2023
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
28 changes: 28 additions & 0 deletions src/Assets/TestProjects/ArtifactsSdkTest/ArtifactsSdkTest.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@

Microsoft Visual Studio Solution File, Format Version 12.00
# Visual Studio Version 17
VisualStudioVersion = 17.0.31903.59
MinimumVisualStudioVersion = 10.0.40219.1
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "PackageReference", "PackageReference\PackageReference.csproj", "{78885455-FD34-4D8A-BB3D-D5DDB1AA2A4F}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "MSBuildSdk", "MSBuildSdk\MSBuildSdk.csproj", "{6BFF7261-0DFB-4C37-954F-DBD11213FDEF}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Release|Any CPU = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
EndGlobalSection
GlobalSection(ProjectConfigurationPlatforms) = postSolution
{78885455-FD34-4D8A-BB3D-D5DDB1AA2A4F}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{78885455-FD34-4D8A-BB3D-D5DDB1AA2A4F}.Debug|Any CPU.Build.0 = Debug|Any CPU
{78885455-FD34-4D8A-BB3D-D5DDB1AA2A4F}.Release|Any CPU.ActiveCfg = Release|Any CPU
{78885455-FD34-4D8A-BB3D-D5DDB1AA2A4F}.Release|Any CPU.Build.0 = Release|Any CPU
{6BFF7261-0DFB-4C37-954F-DBD11213FDEF}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{6BFF7261-0DFB-4C37-954F-DBD11213FDEF}.Debug|Any CPU.Build.0 = Debug|Any CPU
{6BFF7261-0DFB-4C37-954F-DBD11213FDEF}.Release|Any CPU.ActiveCfg = Release|Any CPU
{6BFF7261-0DFB-4C37-954F-DBD11213FDEF}.Release|Any CPU.Build.0 = Release|Any CPU
EndGlobalSection
EndGlobal
5 changes: 5 additions & 0 deletions src/Assets/TestProjects/ArtifactsSdkTest/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<Project>
<PropertyGroup>
<BaseArtifactsPath>$(MSBuildThisFileDirectory)artifacts</BaseArtifactsPath>
</PropertyGroup>
</Project>
6 changes: 6 additions & 0 deletions src/Assets/TestProjects/ArtifactsSdkTest/MSBuildSdk/Class1.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
namespace MSBuildSdk;

public class Class1
{

}
12 changes: 12 additions & 0 deletions src/Assets/TestProjects/ArtifactsSdkTest/MSBuildSdk/MSBuildSdk.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<Project Sdk="Microsoft.NET.Sdk">

<Sdk Name="Microsoft.Build.Artifacts"/>

<PropertyGroup>
<TargetFramework>$(CurrentTargetFramework)</TargetFramework>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
<ArtifactsPath>$(BaseArtifactsPath)\$(MSBuildProjectName)</ArtifactsPath>
</PropertyGroup>

</Project>
14 changes: 14 additions & 0 deletions src/Assets/TestProjects/ArtifactsSdkTest/PackageReference/PackageReference.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>$(CurrentTargetFramework)</TargetFramework>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
<ArtifactsPath>$(BaseArtifactsPath)\$(MSBuildProjectName)</ArtifactsPath>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="Microsoft.Build.Artifacts" Version="4.2.0" />
</ItemGroup>

</Project>
2 changes: 2 additions & 0 deletions src/Assets/TestProjects/ArtifactsSdkTest/PackageReference/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// See https://aka.ms/new-console-template for more information
Console.WriteLine("Hello, World!");
5 changes: 5 additions & 0 deletions src/Assets/TestProjects/ArtifactsSdkTest/global.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"msbuild-sdks": {
"Microsoft.Build.Artifacts": "4.2.0"
}
}
2 changes: 1 addition & 1 deletion src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.DefaultArtifactsPath.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ Copyright (c) .NET Foundation. All rights reserved.
</PropertyGroup>

<!-- Setting ArtifactsPath automatically opts in to the artifacts output format -->
<PropertyGroup Condition="'$(ArtifactsPath)' != ''">
<PropertyGroup Condition="'$(ArtifactsPath)' != '' And '$(UsingMicrosoftArtifactsSdk)' != 'true'">
Copy link
Contributor

Choose a reason for hiding this comment

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

When a user references Microsoft.Build.Artifacts as a package, its .props is imported by .nuget.g.props which is imported after this. So I don't think UsingMicrosoftArtifactsSdk will be set yet since this is imported immediately after Directory.Build.props.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only imported early on if UseArtifactsOutput or ArtifactsPath are set in Directory.Build.props:

sdk/src/Tasks/Microsoft.NET.Build.Tasks/sdk/Sdk.props

Lines 64 to 65 in 9d157dc

<Import Project="$(MSBuildThisFileDirectory)..\targets\Microsoft.NET.DefaultArtifactsPath.props"
Condition="'$(UseArtifactsOutput)' == 'true' Or '$(ArtifactsPath)' != ''"/>

If they are set later on (ie in the project file), then this file will be imported later, during the .targets evaluation: https://github.com/dotnet/sdk/blob/main/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.DefaultOutputPaths.targets#L42-L43

So if the Microsoft.Build.Artifacts NuGet package is being used and ArtifactsPath is set in Directory.Build.props, then the built-in SDK artifacts functionality will be used. If ArtifactsPath is set in the project file (as seems to be suggested in the Microsoft.Build.Artifacts documentation), then the Microsoft.Build.Artifacts logic will be used.

Does this sound OK? Do you know if anyone using Microsoft.Build.Artifacts is setting ArtifactsPath in Directory.Build.props? IE, something like:

<Project>
  <PropertyGroup>
    <BaseArtifactsPath>$(MSBuildThisFileDirectory)artifacts</BaseArtifactsPath>
    <ArtifactsPath>$(BaseArtifactsPath)\$(MSBuildProjectName)</ArtifactsPath>
  </PropertyGroup>
</Project>

Copy link
Member Author

Choose a reason for hiding this comment

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

@jeffkl I went ahead and merged this because we wanted to get the fix in before the .NET 8 Preview 6 branch, Let me know what you think about this and we can follow up with another change if need be.

<UseArtifactsOutput Condition="'$(UseArtifactsOutput)' == ''">true</UseArtifactsOutput>
<IncludeProjectNameInArtifactsPaths Condition="'$(IncludeProjectNameInArtifactsPaths)' == ''">true</IncludeProjectNameInArtifactsPaths>
<_ArtifactsPathLocationType>ExplicitlySpecified</_ArtifactsPathLocationType>
Expand Down
25 changes: 25 additions & 0 deletions src/Tests/Microsoft.NET.Build.Tests/ArtifactsOutputPathTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,31 @@ public void ItErrorsIfUseArtifactsOutputIsSetAndThereIsNoDirectoryBuildProps()
.And
.HaveStdOutContaining("NETSDK1200");
}

[Fact]
public void ItCanBuildWithMicrosoftBuildArtifactsSdk()
{
var testAsset = _testAssetsManager.CopyTestAsset("ArtifactsSdkTest")
.WithSource();

new DotnetBuildCommand(testAsset)
.Execute(Path.Combine(testAsset.Path, "ArtifactsSdkTest.sln"))
.Should()
.Pass();

new DirectoryInfo(Path.Combine(testAsset.Path, "artifacts", "MSBuildSdk", ToolsetInfo.CurrentTargetFramework))
.Should()
.OnlyHaveFiles(new[] { "MSBuildSdk.dll" });

new DirectoryInfo(Path.Combine(testAsset.Path, "artifacts", "PackageReference", ToolsetInfo.CurrentTargetFramework))
.Should()
.OnlyHaveFiles(new[] { "PackageReference.dll", "PackageReference.exe" });

// Verify that default bin and obj folders still exist (which wouldn't be the case if using the .NET SDKs artifacts output functianality
new FileInfo(Path.Combine(testAsset.Path, "MSBuildSdk", "bin", "Debug", ToolsetInfo.CurrentTargetFramework, "MSBuildSdk.dll")).Should().Exist();
new FileInfo(Path.Combine(testAsset.Path, "MSBuildSdk", "obj", "Debug", ToolsetInfo.CurrentTargetFramework, "MSBuildSdk.dll")).Should().Exist();

}
}

namespace ArtifactsTestExtensions
Expand Down
Loading