Warn when NuGetPackageRoot is empty but needed for the toolset compiler #43119
Conversation
jjonescz
commented
Aug 30, 2024
jjonescz
commented
- Follow up on Revert disabling the toolset package in Helix #42860.
- Related to Microsoft.Net.Sdk.Compilers.Toolset download check fails because NuGetPackageRoot is unset in some scenarios #43016.
ghost
added
| </data> | ||
| <!-- The latest message added is WindowsSDKXamlInvalidTfm. Please update this value with each PR to catch parallel PRs both adding a new message --> | ||
| <data name="MicrosoftNetSdkCompilersToolsetRootEmpty" xml:space="preserve"> | ||
| <value>NETSDK1221: NuGetPackageRoot property is empty so package Microsoft.Net.Sdk.Compilers.Toolset cannot be used but it is recommended because your MSBuild and SDK versions are mismatched. Ensure you are building with '/restore /t:Build' and not '/t:Restore;Build'.</value> |
There was a problem hiding this comment.
/t:Restore;Build is bad but it's very non-obvious that it is bad. Do we have documentation covering this that we could also refer to from this error message? @baronfel @marcpopMSFT @nkolev92 @zivkan
There was a problem hiding this comment.
That is a great question. Overall the build check rules are going to start defining a lot of bad msbuild practices. Guessing we will need a bit of a FAQ page to cover them all. I would expect this discussion to be a good fit for that type of page.
There was a problem hiding this comment.
although one day we really, really need to split restore and pack into different docs pages, so I don't expect this URL to be long term permanent.
There was a problem hiding this comment.
We've got a tracking issue for a BuildCheck for this that goes into why it's so subtle and painfully wrong: dotnet/msbuild#9553
There was a problem hiding this comment.
Do we have a FAQ style landing page for build check? Basically a page which is here are all the build check warnings that come in the box, their error codes and why we have them?
There was a problem hiding this comment.
Yeah - @JanKrivanek showed it in Showcase earlier today but all built-in BuildChecks create a link that links to their docs on this page (though it's all aka.ms links so we can change it to more detailed docs as we require them).
| <NETSdkWarning ResourceName="MicrosoftNetSdkCompilersToolsetRootEmpty" | ||
| Condition="'$(_MicrosoftNetSdkCompilersToolsetPackageRootEmpty)' == 'true'" /> |
There was a problem hiding this comment.
The previous error task has a !Exists('$(RoslynTargetsPath)') clause. Should that be mirrored here, or not?
There was a problem hiding this comment.
I don't think that's necessary, even if $(RoslynTargetsPath) exists and $(NuGetPackageRoot) is empty, that feels like a situation which warrants a warning.
There was a problem hiding this comment.
This warning is guarded by $(_NeedsToDownloadMicrosoftNetSdkCompilersToolsetPackage) which means that we are using NuGet here. If we're using NuGet and the $(NuGetPackageRoot) is empty then that seems like a very good case for a warning.
One consideration I made here is that -p:RolynTargetsPath= has been a valid customer scenario for 7+ years now. That still works without warning here even if there is an empty $(NuGetPackageRoot) because it doesn't end up down this code path.
test/Microsoft.NET.Restore.Tests/GivenThatWeWantToUseFrameworkRoslyn.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Daniel Plaisted <[email protected]>
|
Merging now that everyone's greened it up. Thanks @jjonescz. |
5 participants
