arcade icon indicating copy to clipboard operation
arcade copied to clipboard

Remove condition on the import of VisualStudio.targets

Open jasonmalinowski opened this issue 11 months ago • 2 comments

This condition has the side effect of changing the packages that we depend on, depending on the type of MSBuild used to restore packages. This is because VisualStudio.targets itself contains PackageReferences. This can cause all sorts of slowdowns when restore needs to rerun a lot.

DRAFT NOTE: it's quite possible this will break all sorts of things; this condition is six years old, so there's no way to know what might be "fixed" by it.

jasonmalinowski avatar Feb 07 '25 01:02 jasonmalinowski

@jasonmalinowski What's the motivation for removing the condition? Does the VSSDK now support netcore?

mmitche avatar Feb 07 '25 15:02 mmitche

@mmitche: I ran into this condition trying to do write some other targets in a Roslyn repo that depended on the VS SDK targets. Until I found this condition, it meant the VS SDK targets I thought were being imported weren't in a lot of cases, resulting in various obscure error messages in some scenarios.

This is also going to cause restore instability. If you have VS open, VS will restore with these packages, and dotnet build will restore without the packages, and since they can't agree on the actual set of packages, they keep fighting. Run dotnet build with VS open, then it'll restore it's way, causing VS to retrigger it's restore which switches it back over. It breaks incrementality so every dotnet build is a full restore + full build (all while VS is reloading projects and consuming lots of CPU there too.)

I'll be honest I'm not sure if the VS SDK really does support netcore or not -- but years ago we did work to exclude the one specific tool we knew would be problematic, so it's not clear to me if this is still needed. The condition is 6 years old so I'm trying this in draft just to see what breaks if I change it.

jasonmalinowski avatar Feb 07 '25 16:02 jasonmalinowski