search

dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.72k stars 1.07k forks source link

Improve missing targeting pack error handling #2930

Open nguerrera opened 5 years ago

nguerrera commented 5 years ago

Today, in the implementation so far, we have: GenerateErrorForMissingTargetingPacks that is set to false for all design-time builds:

https://github.com/dotnet/sdk/blob/b8c8d680ef807a8d35e3ce1dfceb3e69ca7be394/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.TargetingPackResolution.targets#L151-L155

But we want an error in design-time builds when restore has happened. Should this instead be conditioned on the presence of an assets file + !design-time like other targets?

We also discussed special handling to surface errors from packagedownload of targeting pack to project system.

cc @dsplaisted @davkean

nguerrera commented 5 years ago

IMHO, It's critical that we have a high-ranked error in the error list when the targeting pack could not be found.

davkean commented 5 years ago

@nguerrera I'm thinking about restoring legacy's behavior here; avoid loading project if the targeting pack is missing. We have a bunch of NFW around this - and it's probably easier if we just don't load project.

nguerrera commented 5 years ago

@davkean I need a more complete picture of the proposal. Does this mean that a project could never resolve targeting pack during restore in VS? That sounds very unfortunate to me and an experience downgrade from .NET Core 2.x / .NET Standard 2.x.

I'm also concerned about the implementation. Who would determine the TP is not available and how? Would this be probing in a fixed global location or respect the msbuild sdk resolution for the project, etc.?

nguerrera commented 5 years ago

And there won't be targeting packs for < Core 3.0, Standard 2.1. So this would be conditioned on 3.0, 2.1, but you'd still have the old behaviors for older TFMs? Seems like you'd still get those NFW for older TFMs and so you couldn't make it an invariant that tp is never restored and still would have the NFWs for downlevel projects.

davkean commented 5 years ago

The #2 fault of the project system is an exception thrown by the type resolution service due to missing targeting pack. Every consumer of this information (ResX designer, WinForms/XAML designer, etc) needs to handle "mscorlib" is missing case. Need to decide what is more viable; fixing all downstream consumers and making error experience great, or just avoid loading the project. Yes, we'd still encounter downlevel - but it will eventually go away.

It's very clear to me how I would block the project load and give a reasonable experience, it's unclear to me what it takes to fix all consumers and make a really good error situation if loaded the project. Let's sync up and talk about it.

nguerrera commented 5 years ago

It's very clear to me how I would block the project load and give a reasonable experience.

I don't see this clearly yet. I still want to understand "Who would determine the TP is not available and how".

Every consumer of this information [...] needs to handle "mscorlib" is missing case.

Could this VsTargetFrameworkUniverse be made to succeed instead of throwing in this case and reference a minimalist corelib that ships as part of VS?

nguerrera commented 5 years ago

Indeed, let's sync up on this. :)