dotnet / source-build

A repository to track efforts to produce a source tarball of the .NET Core SDK and all its components
MIT License
266 stars 132 forks source link

nuspec with `any` framework version not handled by PackageSourceGenerator #3361

Open mthalman opened 1 year ago

mthalman commented 1 year ago

The PackageSourceGenerator for SBRP doesn't correctly generate ProjectReferences in the csproj for a package whose nuspec file defines a dependency with the any framework version.

Repro:

.\generate.cmd -p Microsoft.CodeAnalysis.CSharp,2.9.0 -e

You'll notice that the nuspec for that package defines this:

<dependencies>
  <dependency id="Microsoft.CodeAnalysis.Common" version="[2.9.0]" />
</dependencies>

There's no group element with a targetFramework element. This implicitly means the target framework is any. This is the value that will be returned when retrieving the TargetFramework metadata from the package dependency.

This causes the generated csproj to be missing a ProjectReference to the dependency because it fails to match on the target framework here: https://github.com/dotnet/source-build-reference-packages/blob/f9dc7282be60127425b58b40972374cdec0348ed/src/packageSourceGenerator/PackageSourceGeneratorTask/GenerateProject.cs#L75

Continuing the example, this means that Microsoft.CodeAnalysis.CSharp.4.0.1.csproj is generated with no reference to Microsoft.CodeAnalysis.Common.2.9.0.csproj.

dotnet-issue-labeler[bot] commented 1 year ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

ViktorHofer commented 1 year ago

The package source generator fundamentally depends on target frameworks and respective nuspec dependency nodes. I strongly suggest to explore removing the prebuild in the source repository instead of trying to bring a super old package from 2018 in.

Do we know which repository depends on that version of Microsoft.CodeAnalysis.CSharp?

mthalman commented 1 year ago

Do we know which repository depends on that version of Microsoft.CodeAnalysis.CSharp?

This was done as part of https://github.com/dotnet/source-build-reference-packages/pull/595, for arcade.

ViktorHofer commented 1 year ago

Just checked. Arcade should just update MicrosoftCodeAnalysisCSharpVersion to something newer. It doesn't need to be pinned down to such an old version. We had a similar issue / requirement in dotnet/sdk: https://github.com/dotnet/sdk/blob/0961189a48b0763d7cccba637539c876ed29f43f/src/Microsoft.DotNet.ApiSymbolExtensions/Microsoft.DotNet.ApiSymbolExtensions.csproj#L8-L11

We should be able to follow that approach in arcade as well: Use 4.0.1 during the normal build and use the very latest during source build.

ViktorHofer commented 1 year ago

@MichaelSimons based on the above observation, I think we should remove these very old packages from SBRP and upgrade consumers (probably just Arcade). Ideally we would do this in combination with removing netstandard1.x assets from SBRP. I would be surprised if the stack still depend on netstandard1.x assets. By removing netstandard1.x support, we make sure that such old packages won't get added again to SBRP and we will be able to remove all netstandard.library targeting packs aside from the latest (2.0.3).

Related: https://github.com/dotnet/source-build/issues/3350

MichaelSimons commented 1 year ago

We should be able to follow that approach in arcade as well: Use 4.0.1 during the normal build and use the very latest during source build.

In order for this to work, it must be implemented at the repo level. What I mean by that is the repo must have a dependency declared and darc subscription so that the repo's source-build leg builds with the latest. We don't want it to only pick up the latest in the production source-build. This would be a recipe for breaks when the repo flows into the VMR.

In general, I am not a fan of repos conditioning which versions they use based on source-build. It is preferrable for one version to be used IMO.

I think we should remove these very old packages from SBRP and upgrade consumers (probably just Arcade). Ideally we would do this in combination with removing netstandard1.x assets from SBRP

I have no objection to that.

I would be surprised if the stack still depend on netstandard1.x assets.

There are a number still being referenced across the product.