dotnet pack fails using UseArtifactsOutput=true with multi-targeting and global usings

[martincostello]

NuGet Product Used

dotnet.exe

Product Version

8.0.100-preview.4.23260.5

Worked before?

N/A - new functionality

Impact

It's more difficult to complete my work

Repro Steps & Context

Copied from https://github.com/dotnet/sdk/issues/32675#issuecomment-1561010237 as requested by @ViktorHofer

Describe the bug

I'm testing out an internal library with .NET 8 preview 4 and UseArtifactsOutput=true and dotnet pack fails with the following error (as we have treat warnings as errors turned on):

[/runner/_work/LambdaWorker/LambdaWorker/src/LambdaWorker/JustEat.LambdaWorker.csproj::TargetFramework=net7.0]
  JustEat.LambdaWorker -> /runner/_work/LambdaWorker/LambdaWorker/artifacts/bin/JustEat.LambdaWorker/release_net6.0/JustEat.LambdaWorker.dll
  JustEat.LambdaWorker -> /runner/_work/LambdaWorker/LambdaWorker/artifacts/bin/JustEat.LambdaWorker/release_net7.0/JustEat.LambdaWorker.dll
  JustEat.LambdaWorker -> /runner/_work/LambdaWorker/LambdaWorker/artifacts/bin/JustEat.LambdaWorker/release_net8.0/JustEat.LambdaWorker.dll
  Successfully created package '/runner/_work/LambdaWorker/LambdaWorker/artifacts/package/release/JustEat.LambdaWorker.8.0.304-preview.293.nupkg'.
Error: /home/runner/.dotnet/sdk/8.0.100-preview.4.23260.5/Sdks/NuGet.Build.Tasks.Pack/buildCrossTargeting/NuGet.Build.Tasks.Pack.targets(221,5): error NU5118: Warning As Error: File '/runner/_work/LambdaWorker/LambdaWorker/artifacts/obj/JustEat.LambdaWorker/release_net7.0/JustEat.LambdaWorker.GlobalUsings.g.cs' is not added because the package already contains file 'src/LambdaWorker/JustEat.LambdaWorker.GlobalUsings.g.cs' [/runner/_work/LambdaWorker/LambdaWorker/src/LambdaWorker/JustEat.LambdaWorker.csproj]
Error: /home/runner/.dotnet/sdk/8.0.100-preview.4.23260.5/Sdks/NuGet.Build.Tasks.Pack/buildCrossTargeting/NuGet.Build.Tasks.Pack.targets(221,5): error NU5118: Warning As Error: File '/runner/_work/LambdaWorker/LambdaWorker/artifacts/obj/JustEat.LambdaWorker/release_net8.0/JustEat.LambdaWorker.GlobalUsings.g.cs' is not added because the package already contains file 'src/LambdaWorker/JustEat.LambdaWorker.GlobalUsings.g.cs' [/runner/_work/LambdaWorker/LambdaWorker/src/LambdaWorker/JustEat.LambdaWorker.csproj]
  Successfully created package '/runner/_work/LambdaWorker/LambdaWorker/artifacts/package/release/JustEat.LambdaWorker.8.0.304-preview.293.symbols.nupkg'.

dotnet pack seems to be trying to add the same compiler-generating global using files for each TFM to the symbols package and emitting a warning.

To Reproduce

Pending: I haven't yet worked out what the "magic combination" of factors is that causes this problem to make a simple repro. The repository I'm hitting this with is private so I can't share as-is. The issue seems to occur when --include-sources is specified when producing a symbol package.

Verbose Logs

I made a `.binlog` available to @ViktorHofer that either they or I can provide to additional interested parties via email.

[dsplaisted]

This appears to happen when a project is packed under the following conditions:

• The intermediate output path is outside of the project folder (common with UseArtifactsOutput)
• The project is multi-targeted
• IncludeSource is specified

The pack targets call the SourceFilesProjectOutputGroup target for each TargetFramework when IncludeSource is true. The SourceFilesProjectOutputGroup target uses the AssignTargetPath task to try to get the relative path of each source file compared to the project folder. For files outside of the project folder, the task returns the simple filename, which causes a conflict for multi-targeted projects.

Probably if a source file is outside of the project folder, it should try to get its path relative to BaseIntermediateOutputPath.

[nkolev92]

For files outside of the project folder, the task returns the simple filename

@dsplaisted Doesn't this mean that a fix would require changing that target and not just pack?

[dsplaisted]

@nkolev92 We're wary of changing the SourceFilesProjectOutputGroup target, as it has been like that unchanged for a long time. It's not especially complicated though, so I'd suggest creating a new target in the Pack .targets with the changes needed.

[dsplaisted]

@nkolev92 Any update on this? I don't think it's waiting for the customer as indicated by the label.

[nkolev92]

@dsplaisted I'm guessing the artifacts output feature is only in 8.0.1xx?

Trying to understand for when to prioritize an investigation.

[dsplaisted]

Yes, UseArtifactsOutput is a feature in 8.0.1xx. Thanks.

[heng-liu]

I could repro with .NET SDK version 8.0 preview 3 and above. The repro steps are: 1.Create a solution with a project targeting two TFMs 2.Create a Directory.Build.props in the solution folder, with the following content:

<Project>
  <PropertyGroup>
    <UseArtifactsOutput>true</UseArtifactsOutput>
  </PropertyGroup>
</Project>

3.Run dotnet pack .\ClassLibrary9\ClassLibrary9.csproj --include-symbols --include-source There will be an NU5118 warning:

C:\Program Files\dotnet\sdk\8.0.100-preview.4.23260.5\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(221,5): warning NU5118: File 'C:\Repos\ClassLibrary9\artifacts\obj\ClassLibrary9\release_net8.0\ClassLibrary9.GlobalUsings.g.cs
' is not added because the package already contains file 'src\ClassLibrary9\ClassLibrary9.GlobalUsings.g.cs' [C:\Repos\ClassLibrary9\ClassLibrary9\ClassLibrary9.csproj] 

It doesn't repro with dotnet sdk version 8.0 preview 2 and below, as the UseArtifactsOutput is a new feature in .NET SDK 8.0 preview3: https://devblogs.microsoft.com/dotnet/announcing-dotnet-8-preview-3/#simplified-output-path

[heng-liu]

Thanks for @dsplaisted 's previous comments! This issue could be repro when all the 3 conditions are satisfied: (I tried all the combinations of only two conditions below and this warning won’t be raised)

1. The project is multi-targeted
2. Both --include-symbols and --include-source are specified (pack a symbol package with source files inside)
3. The intermediate output path is outside of the project folder (common with UseArtifactsOutput)

[heng-liu]

The binlog shows the GlobalUsings.g.cs files under different TFMs have the same TargetPath, so there will be duplicate file in pack: The reason is when source files are not in project folder (use UseArtifactsOutput to relocate artifacts files), we will just append the file name so there will be no TFM in the target path. Code: https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Commands/MSBuildProjectFactory.cs#L240-L285

[nkolev92]

The real issue here is the target path. When a file is from within the project folder, we include it via src\pathRelativeToFolder.

We can't do that for files outside of the folder, as it'd end up with files outside of the zip, which is zip slip and not allowed in NuGet.

A solution would be replacing .. with something _ and hope that all the other source files don't clash with it.

What this basically means though is that there isn't a "correct" solution. All of this would be dependant on the source file structure, even though our special char is unlikely to clash.

We can use a random char/string? We'd ofc be able to validate NuGet side that clashes are not a thing.

There's also the question whether the extra file structure matters.

We need to understand how these files are getting used in order to come up with the best convention.

[nkolev92]

@martincostello

For .NET projects, the recommended symbosl package type is snupkg. Furthermore you're already embed sources, so --include-source is probably unnecessary.

Now to options for fixing the problem.

I'm not yet certain how the files are used by the debugger and whether we'd need any changes in the debugger to potentially pick up some of these files. I see a few options.

1. Don't include files from outside of the project structure in the symbols package. Realistically, we haven't gotten any feedback about any of these files not working. While the feature is new, having an artifacts folder outside of the project structure isn't.

2. Include the files and adjust for the project structure. Example: /Lib/Class1.cs, /Utility.cs, /shared/Shared.cs Today this would lead to src/Lib/Class1, src/Utility.cs, src/Shared.cs.

We probably need to adjust for the structure somehow. Example: src/shared/shared.cs, src/utility.cs, src/Lib/CLass1.cs

3. Allow stomping to occur (?) and just don't warn? This is least likely to cause issues given that this is how things have always worked.

All this being said, we really need to encourage anyone with .NET projects to use snupkgs.

[martincostello]

Thanks for the write-up.

I believe the current flags we are using have evolved over time to handle our previous internal usage of ProGet to publish and consume private NuGet packages to, where support for newer formats like snupkg was non-existent or limited. IIRC these flags made debugging this private packages work nicely for developers in Visual Studio and Rider. I vaguely remember trying snupkg back when it was new and we ended up reverting as something didn't work well/correctly. Our use of GitHub Enterprise needing auth to download files via SourceLink may also have contributed to this combination of settings.

We've since moved over to Artifactory, so maybe it's time for us to re-evaluate whether or not we can switch over to the same publishing settings we use for our public packages that we push to NuGet.org.

I'll take a look at the original library where this issue came up next week to see if we can just avoid this one by changing the flags we use.

/cc @slang25 as I think it was you who originally found the issue with Rider(?) when we tried out snupkg(?)

[nkolev92]

Doing some digging about how these files are used, there's a chance that symbol servers do some post processing to correlate files and might be rewriting pdbs to achieve that. Ex. http://www.symbolsource.org/Public If that is happening, then the debugger would use https://learn.microsoft.com/en-us/windows-hardware/drivers/debugger/srcsrv?redirectedfrom=MSDN, to download it.

This is not the recommended path anymore, but it's consideration for a fix here. We need to do some testing to see what the symbol server does before we commit to a particular fix.

In the meantime, @martincostello, if you can't switch, you can use WarningsNotAsErrors or NoWarn, the warning code in your failure.