Skip to content

Conversation

tmeschter
Copy link
Contributor

@tmeschter tmeschter commented Nov 13, 2023

The Microsoft.Net.Compilers.Toolset package does not contain libraries to reference; rather, it allows us to pick up different versions of the C# compiler and associated MSBuild files than what we would normally get from the SDK. As a NuGet package, however, it is subject to the same transitive reference rules as every other package and that means that consumers of our Microsoft.VisualStudio.ProjectSystem.Managed NuGet package also end up depending on Microsoft.Net.Compilers.Toolset whether or not they want to.

This means we're changing how consuming projects build in a way that has nothing to do with the functionality they need from Microsoft.VisualStudio.ProjectSystem.Managed which is not what was intended. We see this in other repos using Microsoft.Net.Compilers.Toolset; NuGet complains if they are not using the same version of the package.

Here we fix this by specify that all the assets of the package are private, meaning that they will be used by projects that reference Microsoft.Net.Compilers.Toolset but will not be passed on transitively. We already do this with other packages that are used strictly to modify the build.

Microsoft Reviewers: Open in CodeFlow

The Microsoft.Net.Compilers.Toolset package does not contain libraries to reference; rather, it allows us to pick up different versions of the C# compiler and associated MSBuild files than what we would normally get from the SDK. As a NuGet package, however, it is subject to the same transitive reference rules as every other package and that means that consumers of our Microsoft.VisualStudio.ProjectSystem.Managed NuGet package _also_ end up depending on Microsoft.Net.Compilers.Toolset whether or not they want to.

This means we're changing how consuming projects build in a way that has nothing to do with the functionality they need from Microsoft.VisualStudio.ProjectSystem.Managed which is not what was intended. We see this in other repos using Microsoft.Net.Compilers.Toolset; NuGet complains if they are not using the same version of the package.

Here we fix this by specify that all the assets of the package are private, meaning that they will be used by projects that reference Microsoft.Net.Compilers.Toolset but will not be passed on transitively. We already do this with other packages that are used strictly to modify the build.
@tmeschter tmeschter added the Area-Infrastructure Relates to build, test & run infrastructure of this repo. label Nov 13, 2023
@tmeschter tmeschter requested a review from a team as a code owner November 13, 2023 17:40
@@ -12,7 +12,7 @@
<!-- Path Property: PkgMicrosoft_DiaSymReader_Pdb2Pdb -->
<PackageReference Include="Microsoft.DiaSymReader.Pdb2Pdb" ExcludeAssets="all" GeneratePathProperty="true" />
<PackageReference Include="Microsoft.DotNet.XliffTasks" PrivateAssets="all" />
<PackageReference Include="Microsoft.Net.Compilers.Toolset" />
<PackageReference Include="Microsoft.Net.Compilers.Toolset" PrivateAssets="all" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should re-evaluate whether or not we need this package to begin with. It exists to give you a way to pick up a different version of the compiler than you get with the SDK, and this can be useful if you are stuck with an SDK with a compiler bug or want to use a compiler feature not available through the SDK. However, we don't use lock ourselves to a particular SDK with global.json and frequently have pre-release SDK builds on our systems. As such, the compiler we've chosen here will often be older than the SDK we're using, and sometimes newer, but we're not controlling the relationship between them in any meaningful way.

Copy link
Member

Choose a reason for hiding this comment

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

I like that this package lets us dogfoods new compiler bits in a way that works for all developers who might open this repo. If we leave it to the SDK, then developers on older SDKs won't be able to compile.

Copy link
Member

@drewnoakes drewnoakes left a comment

Choose a reason for hiding this comment

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

Can this be specified on the PackageVersion? It's not necessary for this package, but for others it might ensure all references pick up the same metadata.

@tmeschter
Copy link
Contributor Author

@haileymck I don't think PrivateAssets/IncludeAssets/ExcludeAssets metadata transfers from the PackageVersion to the PackageReference.

@tmeschter
Copy link
Contributor Author

Closing in favor of #9329.

@tmeschter tmeschter closed this Nov 13, 2023
@tmeschter tmeschter reopened this Nov 14, 2023
@tmeschter
Copy link
Contributor Author

Re-opening as there is ongoing discussion RE: #9329.

@tmeschter tmeschter merged commit 5647e29 into dotnet:main Nov 14, 2023
@ghost ghost added this to the 17.9 milestone Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure Relates to build, test & run infrastructure of this repo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants