-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Disable multi-level lookup by default #67022
Conversation
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov Issue DetailsDesign doc: Disable multi-level lookup by default
Actual documentation is not yet written. The links currently redirect to the closest related existing docs I found. Vast majority of this is @vitek-karas' work. @richlander - the current messaging for resolution failures and Framework resolution failureConsole application
Windows GUI applicationSDK resolution failureNo compatible SDK for version in global.json
No SDKs
|
Author: | elinor-fung |
---|---|
Assignees: | - |
Labels: |
|
Milestone: | 7.0.0 |
The host tests use FluentAssertions which has its own implementation of string formatting, for things like `Print("Some {0}", "message\r\nline")`. Instead of taking the string replacement as-is, it formats it by wrapping it in double quotes and replaces newlines with escaped values, so the above would output: ``` Some "message\r\nline" ``` This makes the test output quite hard to read without formatting it back (by unescaping the newlines). Unfortunately FluentAssertions tries hard to always run the value through its own formatter. So even if we preformat everything and instead call `Print($"Some {"message\r\nline"})` it only works sometimes. The string is still parsed and the parser may fail if it finds for example only an open `{` without a closing one. This makes it nest to useless as we would have to make sure to somehow escape everything we feed in. So this change introduces a new extension method FailWithPreformatted which circumvents the automatic formatting and uses the underlying AddFailure method which will not format anything.
The host tests use FluentAssertions which has its own implementation of string formatting, for things like `Print("Some {0}", "message\r\nline")`. Instead of taking the string replacement as-is, it formats it by wrapping it in double quotes and replaces newlines with escaped values, so the above would output: ``` Some "message\r\nline" ``` This makes the test output quite hard to read without formatting it back (by unescaping the newlines). Unfortunately FluentAssertions tries hard to always run the value through its own formatter. So even if we preformat everything and instead call `Print($"Some {"message\r\nline"})` it only works sometimes. The string is still parsed and the parser may fail if it finds for example only an open `{` without a closing one. This makes it nest to useless as we would have to make sure to somehow escape everything we feed in. So this change introduces a new extension method FailWithPreformatted which circumvents the automatic formatting and uses the underlying AddFailure method which will not format anything.
The host tests use FluentAssertions which has its own implementation of string formatting, for things like `Print("Some {0}", "message\r\nline")`. Instead of taking the string replacement as-is, it formats it by wrapping it in double quotes and replaces newlines with escaped values, so the above would output: ``` Some "message\r\nline" ``` This makes the test output quite hard to read without formatting it back (by unescaping the newlines). Unfortunately FluentAssertions tries hard to always run the value through its own formatter. So even if we preformat everything and instead call `Print($"Some {"message\r\nline"})` it only works sometimes. The string is still parsed and the parser may fail if it finds for example only an open `{` without a closing one. This makes it nest to useless as we would have to make sure to somehow escape everything we feed in. So this change introduces a new extension method FailWithPreformatted which circumvents the automatic formatting and uses the underlying AddFailure method which will not format anything.
Update tests to match the new MLL behavior
Looks really good.
How about this small update:
In some cases, "App" would be In cases where apphost cannot find DOTNET_ROOT (or the equivalent), then we could print "Unknown" or "Not found" for ".NET location".
Can we remove that? I don't think it is helpful. If it was only shown when
What happens when one isn't found? I think we should keep this section (not elide) it and instead print "Not found", just like we'd do above for the ".NET location" field. |
Ah, I forgot. We should also show architecture in these error messages. Like this.
It duplicates "Framework" except when it doesn't. |
Updated to always include the With updates to the framework resolution failure message:
|
Opened dotnet/sdk#24541 to remove it. |
FWIW, looking at this without following the issue, I read this as what the app is requesting
and this is clearly what was found:
but what is this part? is it "this is where the framework found is installed" -- because that's already in the output. this needs clarification
|
Is 'framework' an important word to use here? (as opposed eg to 'runtime' or '.NET runtime'). In my head I think, what kind of framework -- oh, it's the .NET framework -- confusion. 'Shared framework' would be unambiguous, but I don't think most app users will know what that is. |
Yes - where 'real' means using default location (not custom registration or private install). The host messages use 'framework' to refer to the defined groupings of libraries like 'framework' is certainly... overloaded. But I think the description we have in the glossary does match our usage. We do have 'shared framework' that glossary, but I'd agree that it is less common and less likely that app users will know what that is (I also don't use it much). We do also try hard to never have |
So if I understand correctly the If not -- it seems it is reasonable to say it is just downloading the "runtime", perhaps |
Yes, it would not always be
|
Got it. OK, my only remaining suggestion is to make it clearer the action required -- these are app users, not developers, eg
|
@richlander - thoughts on #67022 (comment) or something like it ( |
What is the scenario where I can't think of a case where running Regarding the terminology in the errors. Unfortunately the dot.net website uses "runtime" pretty much for everything. It's a ".NET Runtime" or "ASP.NET Core Runtime" or ".NET Desktop Runtime". So I can see the confusion. I think the "error" part should stick to the "framework" term, since that's what is used in the low-level, details version of the docs. But the "action" part may want to use the website terminology - since it leads there. So maybe "To fix this, download .NET runtime from:". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the reported "app path" this looks great. Thanks a lot for all the cleanup and making it all pretty.
I propose "To install missing framework, download:" Very few end-users will ever see this UX, since very few end-users run console apps. I'd like to keep the text as terse as possible. This is the UX that end-users see (borrowed from Elinor above): The UX of the console and GUI are not the same. I'd argue that the proposal makes them as similar as possible. |
I would expect that |
I believe that @vitek-karas' comment is that the message for running - App: D:\.dotnet.install\7.0.100-preview.2.22153.17-win-x64\dotnet.exe
+ App: <full_path_to>\myapp.dll
Architecture: x64
Framework: 'Microsoft.NETCore.App', version '7.0.0' (x64)
.NET location: D:\.dotnet.install\7.0.100-preview.2.22153.17-win-x64\ (And leave running through |
Exactly what @elinor-fung said (I was just too slow to type it). There's little value in showing the path to dotnet in that case (since it's already there anyway), and there's no way to tell what was the app in question (unlike the case with apphost). |
Ah. I didn't pick that up. +1 on that proposal. This is almost frightening. These improvements are going to be quite useful. |
Woohoo. So glad to see this change in. |
Just checking. It doesn't appear that we wrote up a breaking change notice for this. Is that right? |
We have https://docs.microsoft.com/dotnet/core/compatibility/deployment/7.0/multilevel-lookup |
Works. Thanks! |
Design doc: Disable multi-level lookup by default
DOTNET_MULTILEVEL_LOOKUP
environment variabledotnet
. SDKs are only searched for relative to the location ofdotnet
being run.dotnet
commands:--info
,--list-runtimes
,--list-sdks
dotnet
. Runtimes and SDKs are only searched for relative to the location ofdotnet
being run.--info
message includes architecture and new doc link: https://aka.ms/dotnet/runtimes-sdk-infoActual documentation is not yet written. The links currently redirect to the closest related existing docs I found.
Vast majority of this is @vitek-karas' work.
@richlander - the current messaging for resolution failures and
dotnet --info
looks like:Framework resolution failure
Console application
See #67022 (comment)
Windows GUI application
SDK resolution failure
No compatible SDK for version in global.json
No SDKs
dotnet --info
With SDK
No SDKs