Improve error messages when project hasn't been restored for the right target framework or runtime identifier #1262
Conversation
…t wasn't restored
…runtime before calling the compiler
| if (!string.IsNullOrEmpty(runtime)) | ||
| { | ||
| message += " " + string.Format(Strings.AssetsFileMissingRuntimeIdentifier, runtime); | ||
| } |
There was a problem hiding this comment.
We should avoid concatenating messages like this. It makes translation more difficult. It's better to duplicate the fragments to give translators maximum flexibility. I see we have a similar pattern in conflict resolution, but that should be fixed as well.
There was a problem hiding this comment.
At a minimum, if we think duplication is worth avoiding, the concatenation should be done by passing in one fragment into the next via format placeholder. Translators may want to reorder things.
There was a problem hiding this comment.
I've switched to duplication over concatanation.
|
|
||
| The RuntimeIdentifier isn't used to select references to pass to the compiler, so we don't include | ||
| it here. --> | ||
| <CheckForTargetInAssetsFile |
There was a problem hiding this comment.
Where do we check for the case where the assets file simply doesn't exist?
There was a problem hiding this comment.
It will happen the first time a task tries to load the targets file (which will be this task).
| wrong framework, we'd end up finding no references to pass to the compiler, and we'd get a ton of | ||
| compile errors. | ||
|
|
||
| The RuntimeIdentifier isn't used to select references to pass to the compiler, so we don't include |
There was a problem hiding this comment.
When do we verify that the RID has been restored? Or is the existing error behavior good enough?
| <CheckForTargetInAssetsFile | ||
| AssetsFilePath="$(ProjectAssetsFile)" | ||
| TargetFrameworkMoniker="$(TargetFrameworkMoniker)" | ||
| RuntimeIdentifier="" /> |
There was a problem hiding this comment.
Does it make sense to have a RuntimeIdentifier parameter that is never used?
There was a problem hiding this comment.
Originally I was passing the RuntimeIdentifier parameter here. However, that caused the It_passes_ridless_target_to_compiler test to fail. I didn't see an obvious way to keep the test coverage except to not check the RID before the compiler is called.
The GenerateBuildRuntimeConfigurationFiles will check that the right RID has been restored.
I'm not entirely sure whether we should just check everything up-front and remove the It_passes_ridless_target_to_compiler test, or whether there's a better way to get the same coverage.
There was a problem hiding this comment.
I think we should come up with another way to test than to expect that we can pass an invalid rid if we're only compiling. It's not a vaild use case and it would make sense for the check here to be more defensive.
Options:
- A package that causes compilation to fail iff you use its rid-ful assets.
- Use GetValues to check ReferencePath is as we expect heading in to Csc.
There was a problem hiding this comment.
IIRC there often isn't a discernable difference between the values coming from the RIDfull and RIDless targets, which is why I wrote the test this way. It is just some packages that could cause this problem (e.g the one in NuGet/Home#4207, and even that one won't repro anymore because NuGet fixed the underlying bug in NuGet/NuGet.Client#1103). But if you can construct a perverse package that contains bogus entries in the RIDfull targets after restore, then Option 1 is a good replacement for this test.
It is probably OK to temporarily skip the test since my fix and the NuGet fix make this scenario very unlikely.
|
This will also fix #540 |
|
This will also fix #324 |
… impacted by this
|
@eerhardt @nguerrera @natidea OK, I've gone back to checking the RuntimeIdentifier up-front together with the other checks in the assets file. I've rewritten the |
|
@dotnet-bot test Windows_NT_FullFramework Release |
1 similar comment
|
@dotnet-bot test Windows_NT_FullFramework Release |
|
@dotnet-bot test Windows_NT_FullFramework Debug |
|
@MattGertz for approval Customer scenario Trying to build or publish when the project hasn't been restored, or when it hasn't been restored for the right targets. Previously, there was no explicit check for this before the compiler was called, so you would get a bunch of compile errors and possibly an error that told you what was really wrong. This change checks for the right targets in the assets file up front so you will get a single actionable error message. Bugs this fixes: Workarounds, if any Sift through hundreds or thousands of error messages. Risk Low. This moves a check that happened incidentally after the compile was finished to explicitly happen beforehand. Performance impact Low. This is an additional task, but the contents of the lock file are cached between tasks in the same build, so it will still only be loaded once. Is this a regression from a previous update? No Root cause analysis: We weren't explicitly checking for an error condition, and were relying on the fact that something later on would incidentally fail. How was the bug found? Dogfooding |
|
@dotnet-bot test Windows_NT_FullFramework Release |
…209.1 (dotnet#1262) - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.20109.1
Fixes #648
Changes error from:
To either this (if RuntimeIdentifier is empty):
Or this (if there is a RuntimeIdentifier):
This PR also adds an explicit check in the
RunResolvePackageDependenciestarget that the right target exists in the assets file. Previously, it was happening incidentally in theGenerateBuildRuntimeConfigurationFilestarget, which runs after the compiler is called. So the error would likely either not be noticed among all the compile errors, or after #1257 wouldn't be generated at all.