Regression: Pack targets failing - trying to add pdb for embedded type

[rohit21agrawal]

From @onovotny on November 28, 2017 1:48

I’m seeing a regression that’s causing Ix.NET to fail with 2.0.3 and higher. I don’t know when the break was introduced. Nothing in Ix has changed related to this: https://github.com/Reactive-Extensions/Rx.NET/tree/develop/Ix.NET/Source

Getting errors like this: "C:\dev\RxNET\Ix.NET\Source\System.Interactive\System.Interactive.csproj" (pack target) (1) -> (GenerateNuspec target) -> C:\Program Files\dotnet\sdk\2.1.1-preview-007165\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(204,5): err or : The file 'C:\dev\RxNET\Ix.NET\Source\System.Interactive\bin\Release\net45\System.Interactive.pdb' to be packed was not found on disk. [C :\dev\RxNET\Ix.NET\Source\System.Interactive\System.Interactive.csproj]

And the build log shows this:

This is in error because there are no symbols for DebugType = embedded. The build log clearly shows DebugType set to embedded.

msbuild.zip

Copied from original issue: NuGet/Home#6230

[rohit21agrawal]

From @emgarten on November 28, 2017 2:0

@rohit21agrawal can you take a look?

[rohit21agrawal]

I will have to blame msbuild for this because if _GetDebugSymbolsWithTfm target (that resides in NuGet.Build.Tasks.Pack.targets) depends on DebugSymbolsProjectOutputGroup . The target outputs for DebugSymbolsProjectOutputGroup should have been an empty list for DebugType = embedded but instead it points to a non-existent pdb file on disk.

[livarcocc]

I can repro this on 2.0.2 as well, though not on 2.0.0.

[livarcocc]

@rohit21agrawal I see that _GetDebugSymbolsWithTfm was a target that did not exist or at least was not called in the 2.0.0 case where this scenario worked. Is this because it did not exist or is it because there was some input that caused it to no be triggered in 2.0.0? Also, do you have coverage for this scenario on NuGet side?

Just wondering if this is a regression caused by some code change in MSBuild or the SDK or if this is a code change in NuGet that exposed, perhaps, an existing issue in MSBuild or SDK and was never validated until now, by @onovotny.

[rohit21agrawal]

no there has been a code change in nuget to allow pdb files to be packed even when IncludeSymbols is not set (by adding .pdb extension to AllowedOutputExtensionsInBuildOutputFolder) . As a result, _GetDebugSymbolsWithTfm is always called now.

Earlier, we relied on DebugSymbolsProjectOutputGroup target , but only when IncludeSource or IncludeSymbols was set to true.

[clairernovotny]

The Ix.NET code hasn't changed since 2.0. I just haven't been developing that project lately. Did a CI build recently and discovered the failure due to the build agent being updated since before.

[livarcocc]

@onovotny this happens because while building your DebugType is actually set to portable, when we calculate the outputs (you set that in your Directory.Build.props file).

Later on, in your Directory.Build.targets, you set your DebugType to embedded, but I believe at that point the debug symbols have been calculated and pack fails.

I manually set the DebugType to embedded in System.Interactive and pack succeeded. Now, this does not indicate that this isn't a bug in our ordering of things, we need to investigate more to figure that out.

But it seems that it should be possible for you to work around it by setting DebugType. Does this seem reasonable to you.

@rohit21agrawal we need to figure out how to fix the regression, even though I don't believe it is mainstream, given that people who simply set their DebugType to embedded will continue to work.

[livarcocc]

We also should see if it makes sense for the target DebugSymbolsProjectOutputGroup to respect the DebugType set in Directory.Build.targets.

cc @AndyGerlicher

[clairernovotny]

Yes, that workaround worked for now.

[clairernovotny]

To be clear, I didn’t set it to portable in the props….it’s only in the Directory.Build.targets.

Something in the SDK is setting it to portable “for me,” and that may be part of the issue. I remember I had to set DebugType in the Directory.build.targets and not the props because if I used Directory.build.props, my value was overwritten by the SDK’s default values.

[cdmihai]

Setting it in directory.build.props works, and I think that's the best place for this, not in directory.build.targets. Probably got fixed by https://github.com/dotnet/sdk/commit/24db8f1c96dc2857fcecce7f274f1aa94f0017f8

Opened https://github.com/Reactive-Extensions/Rx.NET/pull/451

[clairernovotny]

That's fair and putting in the props is a good solution to this. Still, the errors that can occur if it's in the targets aren't easy to diagnose. Also, I believe the VSTest SDK overrides this in the targets explicitly

I still think it needs a proper fix since having incorrect info on the output group can lead to hard to figure out issues.

[livarcocc]

This has been fixed in the vs test sdk as well.

[clairernovotny]

@livarcocc good to hear, but if someone puts it in the targets, how would they know that is the cause? This doesn't seem that hard to accidently come across.

[cdmihai]

Regarding debuggability, the log only shows the state at the end of evaluation. To look at what happens during evaluation you need to do a /pp build and follow the dependencies.

[clairernovotny]

Point is that people expect to be able to put properties in either props or targets. That this has an incorrect evaluation for the output group seems broken.

[cdmihai]

This brings to question of what's the point of having separate top props imports and bottom targets imports. Historically, I think the intent was to completely separate props from targets. But, if people expect to put properties in both props and targets, then the .net SDK would need to move all its sdk.props logic into sdk.targets. If we do that, might as well get rid of top imports altogether (leave them empty for back compat). And deprecate directory.build.props and sdk.props.

[clairernovotny]

There is a good reason why the DebugType might need to be in the targets -- what if it's a conditional?

Like need to set DebugType to Full or embedded based on the TargetFramework? You can't do that in a props, but you can do that in a targets.

There's also a good reason for props vs targets, since props can default things, the project does what it needs and then the targets can take conditional actions based on the props + project settings.

[cdmihai]

That gives a stronger case why directory.build.props is not that important, in favor of directory.build.targets. You can use the condition argument that you gave for any property or item that gets set in directory.build.props. And I think the implication for this is, that top imports might be useless. In which case we should really move everything from top props imports to bottom targets imports.

[clairernovotny]

But what if I need to set values in my props that are then used in the project file?

[cdmihai]

True, there's that :), One way to do it is to design the all-bottom new SDK logic in such a way that the project file only needs to override values, never read. Otherwise, you can get into impossible situations where you want to specify a property in props so that it can be read in the project file, but at the same time you want to condition it on some SDK computed value.

[clairernovotny]

I guess I always read it as properties are evaluated top-down, before the items are evaluated. So by that reasoning, I should be able to put a property wherever I want and long as things respect values, it'd work.

The tricky part is knowing where the sdk.props/targets fits into the evaluation order.

I don't have the issue number handy, but for my MSBuild.Sdk.Extras package, I don't have the right extensibility points as-is.

I have to tell people to add:

<Import Project="$(MSBuildSDKExtrasTargets)" Condition="Exists('$(MSBuildSDKExtrasTargets)')" /> into their project files because I need my nuget-provided targets to be injected after the project but before the SDK targets. The nuget package provides the property pointing to the location of the targets. I'd love to get rid of that nasty hack.

[cdmihai]

So by that reasoning, I should be able to put a property wherever I want and long as things respect values, it'd work.

What makes it harder is the property dependency graph, and the fact that the final graph gets composed from layering the top_imports_property_graph + project_file_property_graph + bottom_imports_property_graph. This restricts the places where you can put your properties, depending on what you want to happen. It seems that each placement strategy has its drawbacks, which unfortunately forces the user to understand everything.

MSBuild.Sdk.Extras package

I guess the relationship between top and bottom imports is ambiguous, I don't even know it :). Opened #2767. Ideally, it would be nice if you could create an actual MSBuild sdk. Then the order would be more clear <Project Sdk="Microsoft.SDK.Extras;Microsoft.Net.Sdk>"

[clairernovotny]

Ideally, it would be nice if you could create an actual MSBuild sdk.

Yes, there's been an issue over here tracking putting this into the main one https://github.com/dotnet/sdk/pull/889

More importantly, there's no way to pull in an SDK from a NuGet package.

Also, that's still not enough. I need my props to go after the main SDK props and my targets before the main SDK targets. Not first and first.

Basically, I need to consume the defaults/values from the main SDK in my props then provide values before the SDK targets.

Effectively this:

MSbuild SDK Props Extras SDK Props Project Extras targets MSBuild SDK targets

[springy76]

I'm setting <DebugType>embedded</DebugType> via Directory.build.props for a solution containing 70+ projects. On each build approx. 1-5 RANDOM projects fail with this pack error.

I need a robust build system and not a random number generator.

[cdmihai]

@springy76 Can you please open a separate issue with more repro info and diagnostic logs (msbuild /bl) for the failing projects?

[pinkfloydx33]

I know this is an old issue but it's still open. I'm experiencing this in two of my solutions, while four others work fine. Anyone know a workaround to remove the non-existant pdb paths from the copy list?

[ChristoWolf]

I'm also encountering this with a few .NET Standard 2.0 and .NET Framework 4.7.2 projects.

Update: Moving

<PropertyGroup>
    <DebugSymbols>true</DebugSymbols>
    <DebugType>embedded</DebugType>
    <AllowedOutputExtensionsInPackageBuildOutputFolder>$(AllowedOutputExtensionsInPackageBuildOutputFolder);.pdb</AllowedOutputExtensionsInPackageBuildOutputFolder>
</PropertyGroup>

from Directory.Build.targets to Directory.Build.props seems to fix the issue.

So I guess this is related to some required build target being executed before the targets file is injected.

Edit: I just saw that @cdmihai already mentioned this work-around.