CompileDependsOn can't be set in .csproj in SDK style projects

[nvirth]

Issue Description

MsBuild property CompileDependsOn is overridden in SDK style projects here.
Microsoft.Common.CurrentVersion.targets is imported (indirectly) by SDK.targets, which means when using the implicit SDK import style: <Project Sdk="Microsoft.NET.Sdk.Web">, then SDK.targets is the last <Import> in the <Project>, thus Microsoft.Common.CurrentVersion.targets always overrides CompileDependsOn if it is set in a .csproj

Steps to Reproduce

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <CompileDependsOn>MyTarget;$(CompileDependsOn)</CompileDependsOn>
  </PropertyGroup>
</Project>

Using the above in eg a .csproj, property CompileDependsOn won't contain MyTarget.
You can try it out using this prepared demo repository

Expected Behavior

MsBuild property CompileDependsOn should contain MyTarget

Actual Behavior

MsBuild property CompileDependsOn does not contain MyTarget, because CompileDependsOn is overridden later by Microsoft.Common.CurrentVersion.targets

Analysis

MsBuild properties in Microsoft.Common.CurrentVersion.targets are written like "declarations". Like these properties would be created here, but that's not the case, since Microsoft.Common.CurrentVersion.targets is <Import>-ed from SDK.targets - which is implicitely imported as the last <Import> of the <Project> - and not from SDK.props.

Microsoft.Common.CurrentVersion.targets should handle properties like they could be declared already; it should not override these properties, but append to them. Like <CompileDependsOn>MyTarget;$(CompileDependsOn)</CompileDependsOn>

[rainersigwald]

This isn't quite a duplicate of #1680, but is a result of the same thing and generally the recommendations are the same.

[nvirth]

So moving things around in common targets is a very dangerous change, and it won't happen.

What about then changing in Microsoft.Common.CurrentVersion.targets CompileDependsOn (and CleanDependsOn, and the others) like this?

<CompileDependsOn>$(CompileDependsOn);...</CompileDependsOn>

[Forgind]

Team triage: Do you have a scenario in mind that needs this?

[nvirth]

I do have, if this was asked from me

[Forgind]

Can you explain that scenario?

[nvirth]

We have smg like Common.targets, which is <Import>-ed in many .csprojs.
This .targets hooks in custom Targets into CompileDependsOn and CleanDependsOn.
Previously, this Common.targets was only included in old style .csprojs, where it worked fine.
Now, we have new projects using the SDK style format where this does not work by default, only with workarounds.

So, one scenario, where this current github issue causes problems, is upgrading an old format .csproj to the new SDK style .csproj format. Things were just silently missing from the build/clean.

Another one is using a Common.targets in both old and SDK style .csprojs.
It is possible, but we only with workarounds.
When using workarounds, you need to be aware of them, which makes maintenance harder and extension more fragile.

One workaround is using explicit SDK import and ordering Common.target's import later than that. But then it turned out the Common.target just initializes some NuGets, so it has to be imported before the targets delivered by these packages. Which by default just comes with the SDK .targets import. Which has to be before Common.target. To break circular dependencies, import these nugets (using <PackageRefenrece GeneratePathProperty="true"/>) explicitely in the correct place, move them out from the SDK .targets import... This works, but it gets into spaghetti soon.

Another workaround is to move the CompileDependsOn setting in Common.targets into a Target, and call that in InitialTargets.
But then a part of the Properties are set during the evaluation phase, another part of them during the execution phase.
Which works, but is not ideal. You may need to be aware of this fact when extending the .targets files.

[Forgind]

Team triage: Can you try rewriting your hooks to use BeforeTargets or AfterTargets instead?

CompileDependsOn and CleanDependsOn are properties, and their only relevance is their value at the time compile or clean actually execute. If you add your targets to one of them after compile/clean execute, it will be ignored. Similarly, if you add it before but it gets overwritten later, it will be ignored. BeforeTargets and AfterTargets are more reliable.

In particular, Microsoft.Common.CurrentVersion.targets sets an explicit value for CompileDependsOn, so that might explain why your workaround works, but as you said, it quickly turned into spaghetti, as you had other reasons to need to import the SDK .targets first. There may well be something similar in the SDK.

[nvirth]

I will try using AfterTargets="Compile" in future developments.

[Nirmal4G]

Also you can import your Common.targets in Directory.Build.targets. There's also the CustomAfterMicrosoftCommonTargets property. Both would have the same effect as importing after MSBuild targets in the legacy csproj. But only the former can override the targets in implicit NuGet imports. I don't know why anyone mention this?

[benvillalobos]

Closing, feel free to reopen