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

Use PackageDownload in torn builds #41951

Merged
merged 9 commits into from
Jul 12, 2024
Merged

Use PackageDownload in torn builds #41951

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7570bfc
Use PackageDownload in torn builds
jjonescz Jul 2, 2024
080bd3d
Fix texts
jjonescz Jul 9, 2024
9e8feb3
Lowercase NuGet package name
jjonescz Jul 9, 2024
13beb4d
Re-add the warning
jjonescz Jul 9, 2024
23cf26a
Move PackageDownload outside the target
jjonescz Jul 9, 2024
f8fce72
Note UsingTasks duplication in MSBuild
jjonescz Jul 10, 2024
3824f41
Extend build tests
jjonescz Jul 10, 2024
70bd6ed
Fix warning condition
jjonescz Jul 10, 2024
b1840b4
Improve wording
jjonescz Jul 11, 2024
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
13 changes: 0 additions & 13 deletions eng/Publishing.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,19 +88,6 @@
</ItemGroup>
</Target>

<!-- Similarly to FSharp above, Roslyn needs to publish its Framework.Toolset package alongside SDK
so it can be picked up in BuildWithNetFrameworkHostedCompiler scenarios. -->
<PropertyGroup>
<PublishDependsOnTargets>$(PublishDependsOnTargets);_ResolvePublishRoslynNuGetPackages</PublishDependsOnTargets>
</PropertyGroup>

<Target Name="_ResolvePublishRoslynNuGetPackages" Condition="'$(EnableDefaultArtifacts)' == 'true'">
<ItemGroup>
<RoslynPackagesToPush Include="$(NuGetPackageRoot)\Microsoft.Net.Compilers.Toolset.Framework\$(MicrosoftNetCompilersToolsetFrameworkPackageVersion)\*.nupkg" />
<ItemsToPushToBlobFeed Include="@(RoslynPackagesToPush)" IsShipping="true" />
</ItemGroup>
</Target>

<!-- We use a separate target to publish this to blob storage so that we can push this to a relative path inside the blob storage. -->
<Target Name="PublishToolsetAssets" DependsOnTargets="ReadToolsetVersion" BeforeTargets="Publish" Condition="'$(EnableDefaultArtifacts)' == 'true'">
<ItemGroup>
Expand Down
7 changes: 7 additions & 0 deletions sdk.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.WebTools.AspireSe
EndProject
Project("{D954291E-2A0B-460D-934E-DC6B0785DB48}") = "Microsoft.WebTools.AspireService", "src\BuiltInTools\AspireService\Microsoft.WebTools.AspireService.shproj", "{94C8526E-DCC2-442F-9868-3DD0BA2688BE}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Microsoft.Net.Sdk.Compilers.Toolset", "src\Microsoft.Net.Sdk.Compilers.Toolset\Microsoft.Net.Sdk.Compilers.Toolset.csproj", "{FA579C03-2EB4-4D47-88EE-BFF339E96FAF}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand Down Expand Up @@ -971,6 +973,10 @@ Global
{19014C60-F87C-4CC7-AC0F-C41B6126EBCE}.Debug|Any CPU.Build.0 = Debug|Any CPU
{19014C60-F87C-4CC7-AC0F-C41B6126EBCE}.Release|Any CPU.ActiveCfg = Release|Any CPU
{19014C60-F87C-4CC7-AC0F-C41B6126EBCE}.Release|Any CPU.Build.0 = Release|Any CPU
{FA579C03-2EB4-4D47-88EE-BFF339E96FAF}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{FA579C03-2EB4-4D47-88EE-BFF339E96FAF}.Debug|Any CPU.Build.0 = Debug|Any CPU
{FA579C03-2EB4-4D47-88EE-BFF339E96FAF}.Release|Any CPU.ActiveCfg = Release|Any CPU
{FA579C03-2EB4-4D47-88EE-BFF339E96FAF}.Release|Any CPU.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand Down Expand Up @@ -1150,6 +1156,7 @@ Global
{149E3D40-8115-4965-9305-5A1ADBF04899} = {580D1AE7-AA8F-4912-8B76-105594E00B3B}
{19014C60-F87C-4CC7-AC0F-C41B6126EBCE} = {71A9F549-0EB6-41F9-BC16-4A6C5007FC91}
{94C8526E-DCC2-442F-9868-3DD0BA2688BE} = {71A9F549-0EB6-41F9-BC16-4A6C5007FC91}
{FA579C03-2EB4-4D47-88EE-BFF339E96FAF} = {22AB674F-ED91-4FBC-BFEE-8A1E82F9F05E}
EndGlobalSection
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {FB8F26CE-4DE6-433F-B32A-79183020BBD6}
Expand Down
16 changes: 16 additions & 0 deletions src/Microsoft.Net.Sdk.Compilers.Toolset/Microsoft.Net.Sdk.Compilers.Toolset.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>$(SdkTargetFramework)</TargetFramework>
<Description>Transport package for Microsoft.Net.Compilers.Toolset.Framework assemblies. For internal use only.</Description>
<IsPackable>true</IsPackable>
<IncludeBuildOutput>false</IncludeBuildOutput>
<NoPackageAnalysis>true</NoPackageAnalysis>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.Net.Compilers.Toolset.Framework" ExcludeAssets="All" GeneratePathProperty="true" Condition="'$(DotNetBuildSourceOnly)' != 'true'" />
<Content Include="$(PkgMicrosoft_Net_Compilers_Toolset_Framework)\tasks\net472\**\*" PackagePath="%(RecursiveDir)" />
Copy link
Member

Choose a reason for hiding this comment

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

Two follow up items:

  1. Can you also make a change to roslyn which adds some documentation to this package outlining how it is now consumed in the .NET SDK? Essentially a marker that will alert anyone that is trying to change the code in roslyn to what it means in the .NET SDK?
  2. Can you file an issue in roslyn that tracks changing the bootstrap framework CI leg to more closely follow what the .NET SDK is now doing? Essentially set /p:RoslynTargetsPath=...\net472. That will help ensure our test coverage catches any authoring mistakes we make in roslyn.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Note that toolset package layout is hard-coded in SDK roslyn#74301
  2. Change bootstrap framework CI leg to be consistent with SDK roslyn#74303

</ItemGroup>

</Project>
35 changes: 21 additions & 14 deletions src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,24 +220,31 @@ Copyright (c) .NET Foundation. All rights reserved.
</RebuildDependsOn>
</PropertyGroup>

<Target Name="_AddMicrosoftNetCompilerToolsetFrameworkPackage" Condition="'$(MSBuildRuntimeType)' == 'Full'" BeforeTargets="CollectPackageReferences">
<PropertyGroup Condition="'$(MSBuildRuntimeType)' == 'Full'">
<!-- Automatically opt users into using the toolset package if they are running an MSBuild other than what this SDK was built against.
This is to reduce 'tearing'/dependency mismatch, but as always users can override this behavior by disabling the hosted compiler flag. -->
<BuildWithNetFrameworkHostedCompiler Condition="'$(BuildWithNetFrameworkHostedCompiler)' == '' and '$(_IsDisjointMSBuildVersion)' == 'true'">true</BuildWithNetFrameworkHostedCompiler>
</PropertyGroup>

<!-- Users should not be setting Microsoft.Net.Compilers.Toolset.Framework directly.
If they do, and they also set BuildWithNetFrameworkHostedCompiler, we will have
a duplicate PackageReference. This makes it more explicit that that is not supported. -->
<NETSdkWarning ResourceName="CannotDirectlyReferenceMicrosoftNetCompilersToolsetFramework"
Condition="'@(PackageReference->AnyHaveMetadataValue('Identity', 'Microsoft.Net.Compilers.Toolset.Framework'))' == 'true'" />
<PropertyGroup Condition="'$(BuildWithNetFrameworkHostedCompiler)' == 'true' and '$(OS)' == 'Windows_NT'">
<RoslynTargetsPath>$(NuGetPackageRoot)\microsoft.net.sdk.compilers.toolset\$(NETCoreSdkVersion)</RoslynTargetsPath>
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 for PackageDownload, GeneratePathProperty is automatically set to true. Any reason not to use the property NuGet generates for the package?

Also, what happens during the restore phase when the package hasn't been downloaded yet. Is it OK for restore if this points to a non-existent path?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think for PackageDownload, GeneratePathProperty is automatically set to true. Any reason not to use the property NuGet generates for the package?

I think the property is not available for PackageDownload: NuGet/Home#8476

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, what happens during the restore phase when the package hasn't been downloaded yet. Is it OK for restore if this points to a non-existent path?

I think yes, the compiler shouldn't be used during or before restore, right?
Also setting the path after restoring doesn't seem to have any effect, I guess the UsingTask is considered sooner and cannot be overwritten?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, what happens during the restore phase when the package hasn't been downloaded yet. Is it OK for restore if this points to a non-existent path?

Looks like you were right, that is a problem in Visual Studio: #42238

<_NeedToDownloadMicrosoftNetSdkCompilersToolsetPackage>true</_NeedToDownloadMicrosoftNetSdkCompilersToolsetPackage>
</PropertyGroup>

<PropertyGroup>
<!-- Automatically opt users into using the toolset package if they are running an MSBuild other than what this SDK was built against.
This is to reduce 'tearing'/depdendency mismatch, but as always users can override this behavior by disabling the hosted compiler flag. -->
<BuildWithNetFrameworkHostedCompiler Condition="'$(BuildWithNetFrameworkHostedCompiler)' == '' and '$(_IsDisjointMSBuildVersion)' == 'true'">true</BuildWithNetFrameworkHostedCompiler>
</PropertyGroup>
<UsingTask TaskName="Microsoft.CodeAnalysis.BuildTasks.CopyRefAssembly" AssemblyFile="$(RoslynTargetsPath)\Microsoft.Build.Tasks.CodeAnalysis.dll" />
Copy link
Member

Choose a reason for hiding this comment

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

@rainersigwald should we change common.tasks in msbuild to have a comment that says more less "if you change this lest, you also need to change this file in the .NET SDK"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added a note here and to msbuild: dotnet/msbuild#10351

<UsingTask TaskName="Microsoft.CodeAnalysis.BuildTasks.Csc" AssemblyFile="$(RoslynTargetsPath)\Microsoft.Build.Tasks.CodeAnalysis.dll" />
<UsingTask TaskName="Microsoft.CodeAnalysis.BuildTasks.Vbc" AssemblyFile="$(RoslynTargetsPath)\Microsoft.Build.Tasks.CodeAnalysis.dll" />

<ItemGroup Condition="'$(BuildWithNetFrameworkHostedCompiler)' == 'true' and '$(OS)' == 'Windows_NT' ">
<PackageReference Include="Microsoft.Net.Compilers.Toolset.Framework" PrivateAssets="all" Version="$(_NetFrameworkHostedCompilersVersion)" IsImplicitlyDefined="true" />
</ItemGroup>
<ItemGroup Condition="'$(_NeedToDownloadMicrosoftNetSdkCompilersToolsetPackage)' == 'true'">
<PackageDownload Include="Microsoft.Net.Sdk.Compilers.Toolset" Version="[$(NETCoreSdkVersion)]" />
</ItemGroup>

<Target Name="_CheckMicrosoftNetSdkCompilersToolsetPackageReference" Condition="'$(_NeedToDownloadMicrosoftNetSdkCompilersToolsetPackage)' == 'true'" BeforeTargets="CollectPackageReferences">
<!-- Users should not be setting Microsoft.Net.Compilers.Toolset.Framework directly.
If they do, and BuildWithNetFrameworkHostedCompiler is also set, the former will override the latter.
This makes it more explicit that that is not supported. -->
<NETSdkWarning ResourceName="CannotDirectlyReferenceMicrosoftNetCompilersToolsetFramework"
Condition="'@(PackageReference->AnyHaveMetadataValue('Identity', 'Microsoft.Net.Compilers.Toolset.Framework'))' == 'true'" />
</Target>

<!-- TODO: this target should not check GeneratePackageOnBuild.
Expand Down
43 changes: 16 additions & 27 deletions test/Microsoft.NET.Restore.Tests/GivenThatWeWantToUseFrameworkRoslyn.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ public GivenThatWeWantToUseFrameworkRoslyn(ITestOutputHelper log) : base(log)
}

[FullMSBuildOnlyFact]
public void It_restores_Microsoft_Net_Compilers_Toolset_Framework_when_requested()
public void It_downloads_Microsoft_Net_Compilers_Toolset_Framework_when_requested()
{
const string testProjectName = "NetCoreApp";
var project = new TestProject
Expand All @@ -24,21 +24,19 @@ public void It_restores_Microsoft_Net_Compilers_Toolset_Framework_when_requested
var testAsset = _testAssetsManager
Copy link
Member

Choose a reason for hiding this comment

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

Can't comment on it but then we should change the name of this test now to It_downloads...

.CreateTestProject(project);

string projectAssetsJsonPath = Path.Combine(
testAsset.Path,
project.Name,
"obj",
"project.assets.json");
NuGetConfigWriter.Write(testAsset.Path, TestContext.Current.TestPackages);

var restoreCommand =
testAsset.GetRestoreCommand(Log, relativePath: testProjectName);
restoreCommand.Execute().Should().Pass();
var customPackageDir = Path.Combine(testAsset.Path, "nuget-packages");

Assert.Contains("Microsoft.Net.Compilers.Toolset.Framework", File.ReadAllText(projectAssetsJsonPath));
testAsset.GetRestoreCommand(Log, relativePath: testProjectName)
.WithEnvironmentVariable("NUGET_PACKAGES", customPackageDir)
.Execute().Should().Pass();

Assert.True(Directory.Exists(Path.Combine(customPackageDir, "microsoft.net.sdk.compilers.toolset")));
}

[FullMSBuildOnlyFact]
public void It_restores_Microsoft_Net_Compilers_Toolset_Framework_when_MSBuild_is_torn()
public void It_downloads_Microsoft_Net_Compilers_Toolset_Framework_when_MSBuild_is_torn()
{
const string testProjectName = "NetCoreApp";
var project = new TestProject
Expand All @@ -53,24 +51,15 @@ public void It_restores_Microsoft_Net_Compilers_Toolset_Framework_when_MSBuild_i
var testAsset = _testAssetsManager
Copy link
Member

Choose a reason for hiding this comment

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

Same comment about name change

.CreateTestProject(project);

string projectAssetsJsonPath = Path.Combine(
testAsset.Path,
project.Name,
"obj",
"project.assets.json");
NuGetConfigWriter.Write(testAsset.Path, TestContext.Current.TestPackages);

var restoreCommand =
testAsset.GetRestoreCommand(Log, relativePath: testProjectName);
restoreCommand.Execute().Should().Pass();
var customPackageDir = Path.Combine(testAsset.Path, "nuget-packages");

if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
Assert.Contains("Microsoft.Net.Compilers.Toolset.Framework", File.ReadAllText(projectAssetsJsonPath));
}
else
{
Assert.DoesNotContain("Microsoft.Net.Compilers.Toolset.Framework", File.ReadAllText(projectAssetsJsonPath));
}
testAsset.GetRestoreCommand(Log, relativePath: testProjectName)
.WithEnvironmentVariable("NUGET_PACKAGES", customPackageDir)
.Execute().Should().Pass();

Assert.True(Directory.Exists(Path.Combine(customPackageDir, "microsoft.net.sdk.compilers.toolset")));
}

[FullMSBuildOnlyFact]
Expand Down
5 changes: 5 additions & 0 deletions test/Microsoft.NET.TestFramework/BuildTestPackages.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,11 @@
<Version>1.0.0</Version>
<Clean>True</Clean>
</BaseTestPackageProject>
<BaseTestPackageProject Include="src/Microsoft.Net.Sdk.Compilers.Toolset">
<Name>Microsoft.Net.Sdk.Compilers.Toolset</Name>
<ProjectName>Microsoft.Net.Sdk.Compilers.Toolset.csproj</ProjectName>
<Version>$(Version)</Version>
</BaseTestPackageProject>

<BaseTestPackageProject>
<NuPkgName Condition=" '%(NuPkgName)' == '' ">%(Name)</NuPkgName>
Expand Down
Loading