Use PackageDownload in torn builds #41951

jjonescz · 2024-07-03T12:03:27Z

Resolves #41791.

RikkiGibson · 2024-07-08T18:17:30Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets

-  <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'/depdendency mismatch, but as always users can override this behavior by disabling the hosted compiler flag. -->


Suggested change

This is to reduce 'tearing'/depdendency mismatch, but as always users can override this behavior by disabling the hosted compiler flag. -->

This is to reduce 'tearing'/dependency mismatch, but as always users can override this behavior by disabling the hosted compiler flag. -->

jaredpar · 2024-07-08T21:53:17Z

src/Microsoft.Net.Sdk.Compilers.Toolset/Microsoft.Net.Sdk.Compilers.Toolset.csproj

+
+  <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)" />


Two follow up items:

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?

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.

Note that toolset package layout is hard-coded in SDK roslyn#74301

Change bootstrap framework CI leg to be consistent with SDK roslyn#74303

jaredpar · 2024-07-08T21:56:04Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets

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


The way this is written means that if a customer both

Sets BuildWitHNetFrameworkHostedCompiler or builds in a torn SDK

Sets RoslynTargetsPath

Then we will simply ignore (2).

Is that the behavior that we want here? To me that seems reasonable but I also worry about confusing users if they set both and we are silently overwriting one of them. Should we issue a diagnostic here if both are set?

Sounds reasonable, I've tried adding this warning, but I don't know how to reliably detect whether RoslynTargetsPath is set by the user or it has the default value set by msbuild.

Hmm ... that is a good point. That would probably need a msbuild analyzer. Let's skip this for now, can revisit if it becomes a real problem.

Is RoslynTargetsPath meant to be set by customers? I see a few hundred times across all of GH. This seems like something that should have been a private variable originally.

jaredpar · 2024-07-08T21:56:56Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets

-           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" />


@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"?

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

jaredpar · 2024-07-08T22:08:07Z

test/Microsoft.NET.Restore.Tests/GivenThatWeWantToUseFrameworkRoslyn.cs

@@ -24,17 +24,15 @@ public void It_restores_Microsoft_Net_Compilers_Toolset_Framework_when_requested
            var testAsset = _testAssetsManager


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

jaredpar · 2024-07-08T22:08:31Z

test/Microsoft.NET.Restore.Tests/GivenThatWeWantToUseFrameworkRoslyn.cs

@@ -53,28 +51,19 @@ public void It_restores_Microsoft_Net_Compilers_Toolset_Framework_when_MSBuild_i
            var testAsset = _testAssetsManager


Same comment about name change

jjonescz · 2024-07-09T07:32:59Z

src/Tasks/Common/Resources/Strings.resx

-  <data name="CannotDirectlyReferenceMicrosoftNetCompilersToolsetFramework" xml:space="preserve">
-    <value>NETSDK1205: The Microsoft.Net.Compilers.Toolset.Framework package should not be set directly. Set the property 'BuildWithNetFrameworkHostedCompiler' to 'true' instead if you need it.</value>
-    <comment>{StrBegin="NETSDK1205: "}{Locked="Microsoft.Net.Compilers.Toolset.Framework"}{Locked="BuildWithNetFrameworkHostedCompiler"}</comment>
-  </data>


@marcpopMSFT had a comment in a Teams thread about this - instead of removing this warning, should that be changed to warn customers if they reference the new Microsoft.Net.Sdk.Compilers.Toolset package?

We could do that, but nothing happens when customers reference the new package as it has a non-standard layout. So, a warning seems unnecessary to me.

A related question is what happens when a customer references the Microsoft.Net.Compilers.Toolset.Framework package (which we used to warn about). From local testing, it looks like the compiler from the explicit PackageReference takes precedence over the built-in disjoint detection and the PackageDownload.

We can always remove a warning but adding one is a compat break, so I think I lean toward changing it rather than removing it.

If referencing the old package takes precedents over the packagedownload, do we want to keep the original warning in place? I'm less worried now about the sdk package if referencing it doesn't do anything. CC @baronfel as this is one of those situations where a customer could do bad things to themselves but how much do we want to prevent/warn.

jaredpar · 2024-07-09T14:06:29Z

Are there any tests where there is a full end to end test of the system that includes a build? Basically want to exercise the entire code path include executing the compiler. Looking through the PR I didn't see you modify any, but it's possible that they could've been added by our earlier work in torn sdks.

jjonescz · 2024-07-09T14:30:00Z

Are there any tests where there is a full end to end test of the system that includes a build?

I don't think there are existing end-to-end tests, I can look into adding one, but perhaps as a follow up PR, I don't know how complicated that will be.

jaredpar · 2024-07-09T15:19:10Z

I don't think there are existing end-to-end tests, I can look into adding one, but perhaps as a follow up PR, I don't know how complicated that will be.

Fine with it being a follow up PR. I'm not 100% sure if we can do an end-to-end test here or if we have to move that to installer.

@baronfel can you help with some guidance here?

rainersigwald · 2024-07-09T16:00:12Z

src/Tasks/Common/Resources/Strings.resx

-  <data name="CannotDirectlyReferenceMicrosoftNetCompilersToolsetFramework" xml:space="preserve">
-    <value>NETSDK1205: The Microsoft.Net.Compilers.Toolset.Framework package should not be set directly. Set the property 'BuildWithNetFrameworkHostedCompiler' to 'true' instead if you need it.</value>
-    <comment>{StrBegin="NETSDK1205: "}{Locked="Microsoft.Net.Compilers.Toolset.Framework"}{Locked="BuildWithNetFrameworkHostedCompiler"}</comment>
-  </data>


We can always remove a warning but adding one is a compat break, so I think I lean toward changing it rather than removing it.

rainersigwald · 2024-07-09T16:05:04Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets

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


Doesn't NuGet lower-case the package name by default? I guess it won't matter since the only case we care about here is Windows but I'd still do

Suggested change

<RoslynTargetsPath>$(NuGetPackageRoot)\Microsoft.Net.Sdk.Compilers.Toolset\$(NETCoreSdkVersion)</RoslynTargetsPath>

<RoslynTargetsPath>$(NuGetPackageRoot)\microsoft.net.sdk.compilers.toolset\$(NETCoreSdkVersion)</RoslynTargetsPath>

rainersigwald · 2024-07-09T16:12:39Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets


-    <ItemGroup Condition="'$(BuildWithNetFrameworkHostedCompiler)' == 'true' and '$(OS)' == 'Windows_NT' ">
-      <PackageReference Include="Microsoft.Net.Compilers.Toolset.Framework" PrivateAssets="all" Version="$(_NetFrameworkHostedCompilersVersion)" IsImplicitlyDefined="true" />
+  <Target Name="_DownloadMicrosoftNetSdkCompilersToolsetPackage" Condition="'$(_NeedToDownloadMicrosoftNetSdkCompilersToolsetPackage)' == 'true'" BeforeTargets="CollectPackageReferences">


Does this need to be in a target? I think it was before so that the warning could be thrown--is that right @Forgind?

Right, makes sense, it probably doesn't need to be in a Target. Currently it looks like we will be re-adding the warning based on #41951 (comment) - but I guess the PackageDownload could still be outside the Target.

marcpopMSFT · 2024-07-09T16:58:07Z

I don't think there are existing end-to-end tests, I can look into adding one, but perhaps as a follow up PR, I don't know how complicated that will be.

Fine with it being a follow up PR. I'm not 100% sure if we can do an end-to-end test here or if we have to move that to installer.

There's no installer anymore. We do have E2E tests here but @MiYanni is still getting them lined up in the sdk test infrastructure and using the right layout. It might be possible to add a test there but I don't know how you validate as we don't have tight control over the VS version on the images (so you'd be setting the property explicitly but the behavior wouldn't be much different for an E2E test)

jaredpar · 2024-07-09T19:02:47Z

It might be possible to add a test there but I don't know how you validate as we don't have tight control over the VS version on the images (so you'd be setting the property explicitly but the behavior wouldn't be much different for an E2E test)

I'm less concerned about the VS version and more concerned about basic functionality concerns. My idea for a test is more / less to do the following:

> dotnet new webapp
> msbuild -p:BuildWithNetFrameworkHostedCompiler=true -bl

Then ensure the build succeeded and it downloaded + used the compiler from the downloaded package.

There is a bit of risk in constructing the components of this solution in multiple repos. This test would guard against silly mistakes in roslyn thta break the layout of the package, or build authoring errors that messed up the data flow to use the <PcakageDownload> path.

jjonescz · 2024-07-09T19:08:54Z

Then ensure the build succeeded and it downloaded + used the compiler from the downloaded package.

I think the current tests like It_downloads_Microsoft_Net_Compilers_Toolset_Framework_when_requested verify that. E.g., if the layout broke the build inside the test should fail because the UsingTask would have wrong path. The test also verifies the package is downloaded.

dsplaisted · 2024-07-09T19:43:38Z

> msbuild -p:BuildWithNetFrameworkHostedCompiler=true -bl

This is pretty easy to do, something like:

new BuildCommand(testAsset)
    .Execute("-p:BuildWithNetFrameworkHostedCompiler=true")
    .Should()
    .Pass();

> dotnet new webapp

You can do this too, though most of our tests construct the project themselves or use checked-in assets. It's a question of whether you want to depend on the template and have the test use the latest version of the template (and potentially break because of changes to it).

jaredpar · 2024-07-09T19:52:42Z

It's a question of whether you want to depend on the template and have the test use the latest version of the template (and potentially break because of changes to it).

That's exactly what we want. This feature is about "can we build latest" so want it to ebb and flow with how the product evolves.

This is pretty easy to do, something like:

Can that capture the binary log too?

dsplaisted · 2024-07-09T20:05:17Z

Can that capture the binary log too?

You can add -bl as a parameter, which helps when debugging locally, but I don't think we have an easy way hooked up to let you access a binlog that a test creates when running in CI.

dsplaisted · 2024-07-09T20:15:13Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets

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


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?

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

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?

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

marcpopMSFT · 2024-07-09T21:38:54Z

Can that capture the binary log too?

You can add -bl as a parameter, which helps when debugging locally, but I don't think we have an easy way hooked up to let you access a binlog that a test creates when running in CI.

May I suggest using -getItem: or -getProperty? That would let you pull values out of the binlog if you needed them if there was something in an item or property you were looking for.

jaredpar · 2024-07-09T23:09:15Z

That would let you pull values out of the binlog if you needed them if there was something in an item or property you were looking for.

There are two key items we are looking for:

Did the compilation succeed?
Which compiler was used?

Those are really easy to get out of the binlog if we have access to it.

marcpopMSFT · 2024-07-09T23:53:02Z

My point is that we don't have CLI libraries for automating binlog consumption. My recommendation is to do a build per Daniel above and check that it passed and then do a build with the getProperty or getItem flag to get the version of the compiler without having to process a binlog at all.

jjonescz · 2024-07-10T09:15:34Z

May I suggest using -getItem: or -getProperty?

I don't know what item or property to get to determine the used compiler assembly path. We could get the RoslynTargetsPath property but that doesn't guarantee it ends up being the DLL used. I think I can instead just assert the path is in the standard output which contains the full command line executed.

dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Jul 3, 2024

jjonescz force-pushed the tearing branch 2 times, most recently from 31d49bb to 1286ed0 Compare July 3, 2024 16:46

build-analysis bot mentioned this pull request Jul 3, 2024

VMR CI build failures during restore: feed download failed #41590

Open

jjonescz force-pushed the tearing branch from 1286ed0 to 9282748 Compare July 4, 2024 09:45

Use PackageDownload in torn builds

7570bfc

jjonescz force-pushed the tearing branch from 9282748 to 7570bfc Compare July 6, 2024 08:15

jjonescz marked this pull request as ready for review July 6, 2024 13:50

jjonescz requested review from jaredpar, baronfel, rainersigwald and marcpopMSFT July 6, 2024 13:50

jjonescz mentioned this pull request Jul 8, 2024

Fix detection of torn SDK #41907

Merged

chsienki approved these changes Jul 8, 2024

View reviewed changes

RikkiGibson reviewed Jul 8, 2024

View reviewed changes

jaredpar reviewed Jul 8, 2024

View reviewed changes

marcpopMSFT requested a review from dsplaisted July 8, 2024 23:38

jjonescz commented Jul 9, 2024

View reviewed changes

Fix texts

080bd3d

jjonescz mentioned this pull request Jul 9, 2024

Change bootstrap framework CI leg to be consistent with SDK dotnet/roslyn#74303

Open

This was referenced Jul 9, 2024

msbuild crashes with "MSB0001: Internal MSBuild Error: must be valid" dotnet/dnceng#3304

Open

Microsoft.NET.Build.Containers.IntegrationTests are pulling from Docker Hub #42026

Open

jjonescz requested a review from jaredpar July 9, 2024 12:33

jaredpar approved these changes Jul 9, 2024

View reviewed changes

rainersigwald reviewed Jul 9, 2024

View reviewed changes

jjonescz added 3 commits July 9, 2024 19:21

Lowercase NuGet package name

9e8feb3

Re-add the warning

13beb4d

Move PackageDownload outside the target

23cf26a

jjonescz requested a review from rainersigwald July 9, 2024 18:10

build-analysis bot mentioned this pull request Jul 9, 2024

Roslyn analyzer throws error AD0001 NullReferenceException dotnet/dnceng#3305

Open

3 tasks

dsplaisted approved these changes Jul 9, 2024

View reviewed changes

build-analysis bot mentioned this pull request Jul 9, 2024

The "RestoreTask" task returned false but did not log an error dotnet/dnceng#3100

Open

3 tasks

Note UsingTasks duplication in MSBuild

f8fce72

Extend build tests

3824f41

jjonescz mentioned this pull request Jul 10, 2024

Note Roslyn UsingTasks duplication in SDK dotnet/msbuild#10351

Merged

Fix warning condition

70bd6ed

marcpopMSFT mentioned this pull request Jul 10, 2024

Address analyzer and generator torn state with Visual Studio #42087

Open

Improve wording

b1840b4

rainersigwald approved these changes Jul 11, 2024

View reviewed changes

jjonescz merged commit 25cdd7a into dotnet:main Jul 12, 2024
37 checks passed

jjonescz deleted the tearing branch July 12, 2024 06:59

MiYanni mentioned this pull request Jul 12, 2024

Fix TestPackage paths #42136

Merged

	This is to reduce 'tearing'/depdendency mismatch, but as always users can override this behavior by disabling the hosted compiler flag. -->
	This is to reduce 'tearing'/dependency mismatch, but as always users can override this behavior by disabling the hosted compiler flag. -->

		@@ -24,17 +24,15 @@ public void It_restores_Microsoft_Net_Compilers_Toolset_Framework_when_requested
		var testAsset = _testAssetsManager

		@@ -53,28 +51,19 @@ public void It_restores_Microsoft_Net_Compilers_Toolset_Framework_when_MSBuild_i
		var testAsset = _testAssetsManager

	<RoslynTargetsPath>$(NuGetPackageRoot)\Microsoft.Net.Sdk.Compilers.Toolset\$(NETCoreSdkVersion)</RoslynTargetsPath>
	<RoslynTargetsPath>$(NuGetPackageRoot)\microsoft.net.sdk.compilers.toolset\$(NETCoreSdkVersion)</RoslynTargetsPath>

Use PackageDownload in torn builds #41951

Use PackageDownload in torn builds #41951

Conversation

jjonescz commented Jul 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Jul 9, 2024

jjonescz commented Jul 9, 2024

jaredpar commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcpopMSFT commented Jul 9, 2024 • edited by jjonescz Loading

jaredpar commented Jul 9, 2024 • edited Loading

jjonescz commented Jul 9, 2024

dsplaisted commented Jul 9, 2024

jaredpar commented Jul 9, 2024

dsplaisted commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcpopMSFT commented Jul 9, 2024

jaredpar commented Jul 9, 2024

marcpopMSFT commented Jul 9, 2024

jjonescz commented Jul 10, 2024 • edited Loading

marcpopMSFT commented Jul 9, 2024 •

edited by jjonescz

Loading

jaredpar commented Jul 9, 2024 •

edited

Loading

jjonescz commented Jul 10, 2024 •

edited

Loading