Do not throw an InvalidProjectFileException if ProjectLoadSettings.IgnoreMissingImports is specified #2991
Conversation
…noreMissingImports is specified Related to dotnet/project-system#3240
|
fyi @jviau |
|
|
||
| _evaluationLoggingContext.LogBuildEvent(eventArgs); | ||
|
|
||
| projects = new List<ProjectRootElement>(); |
There was a problem hiding this comment.
This line confuses me, but seems correct from context.
There was a problem hiding this comment.
Yeah, projects is an out parameter so it must be set before returning...
| BuildEventContext = _evaluationLoggingContext.BuildEventContext, | ||
| UnexpandedProject = importElement.Project, | ||
| ProjectFile = importElement.ContainingProject.FullPath, | ||
| ImportedProjectFile = null, |
There was a problem hiding this comment.
Currently I use ImportedProjectFile to track imports that were ignored, so I can attempt reloads as they change (in case they become valid). Can we get 2 separate events, one for sdk.props and one for sdk.targets?
There was a problem hiding this comment.
The event will be logged twice but since no project was imported then ImportedProjectFile is null. UnexandedProject is Sdk.props and Sdk.targets. Will that work?
There was a problem hiding this comment.
Tehavior we saw when trying things out was that Visual Studio unloads a project and when you try to reload it you get an error. An SDK not found with IgnoreMissingImports would evaluate to this:
<Project>
<PropertyGroup>
<TargetFramework>netstandard1.3</TargetFramework>
</PropertyGroup>
</Project>And you get this when you save it, VS unloads it, and you try to reload it.
Are you saying you would leave the project loaded and then attempt to reload it if the user fixed it?
There was a problem hiding this comment.
When I added IgnoreInvalidImport I refined the contract for ImportedProjectFile. It should only be null if the condition is false or it is a glob with no matches. This is a case I didn't consider - import is ignored, but the full path is still unknown (because any resolver could eventually supply it). I will have to make a change to CPS to handle this.
Unless you want to set the full path as what the default SDK resolver would supply - the SDK directory under msbuild. Not sure if that is the correct behavior though.
There was a problem hiding this comment.
The default SDK resolver only returns the path if it exists so the Evaluator would not know the value. The only options are:
- Use the value in the Project attribute,
Sdk.propsandSdk.targets - Use
nullsince a path could not be determined
There was a problem hiding this comment.
Maybe there's a difference between a "core" SDK like m.net.sdk(.web) and a custom one not being resolved? As long as you don't mess around with that core sdk and only have a broken reference to a NuGet one, does it still show that error dialog?
There was a problem hiding this comment.
@dasMulli correct, if another non "core" SDK is invalid and ignored, you will not get a dialog. You still get a build error which is what we want but VS does not unload the project
There was a problem hiding this comment.
I am not sure CPS will be able to tell if a "core" SDK (or really any SDK) has broken at the time of reload. It is far too expensive to re-evaluate the project during the reload. Comparing if any Sdk elements or attributes changed won't be enough either, as they can be evaluated - ie: <Sdk Name="$(MySdk)" Version="$(SdkVersion)" />
Letting the error dialog @jeffkl showed above display isn't ideal either, as it would be 1 per project. So if an Sdk came from a shared import, you would see multiple dialogs show up.
There was a problem hiding this comment.
I wonder if this now meets the bar for 15.6 since we will have to make a considerable change to CPS as well.
The experience when breaking the core SDK isn't very good in 15.5 either, but this fix would help a lot when tinkering with custom SDKs, which is a feature in 15.6 that encourages users to modify SDK imports. If some form of a fix that doesn't make me click through hundreds (=6 ^^) of dialogs when I break something in the custom import (or on the NuGet feed) makes it into 15.6.*, that would be awesome!!
For reference, this happens when I break the core SDK in 15.5 (after which the project is just unloaded):
There was a problem hiding this comment.
I agree that experience is awful. To get a good experience we need to do a lot of validation of what has changed in the project during a reload, so that we never reload into a broken "core" SDK. This is very expensive to do, especially when a common import is changed (we have to validate every project that imports it). I am thinking we will have to do something like track every property that is impacting the final SDK result, and if any of them change, we submit an error to the error list asking the user to eventually reload the solution.
|
Hmm Jenkins seems to be using the netci.groovy from master instead of my branch so the build on Mac is failing. |
|
I don't understand why it's running both flavors of macOS CI job. But since the correct one passed I'm ok with moving forward (pending shiproom approval). |
|
When is this going to be inserted to 15.6? CPS will need changes as well, the |
|
@AndyGerlicher was working on an insertion today |
InvalidProjectFileExceptions cause modal dialogs in Visual Studio which so we should respect
ProjectLoadSettings.Related to dotnet/project-system#3240