Improve apphost missing fxr error message with self registered location#6546
Conversation
With the recent changes apphost now looks for shared framework in 3 potential places: the default global location, the globally registered location and the DOTNET_ROOT path. The error message didn't mention the globally registered location, so add that. Required a bit of refactoring in the pals to avoid code duplication.
eerhardt
left a comment
eerhardt
left a comment
There was a problem hiding this comment.
Looks good to me. Just some questions and minor comments.
| pal::get_dotnet_self_registered_config_location(&self_registered_config_location); | ||
| pal::string_t self_registered_message = | ||
| self_registered_config_location.length() > 0 ? | ||
| pal::string_t(_X(" or register the runtime location in [") + self_registered_config_location + _X("]")) : |
There was a problem hiding this comment.
What does register the runtime location mean? Should this instead be:
"or install the runtime in the globally registered location [") + self_registered_config_location + _X("]"
?
There was a problem hiding this comment.
If there is a globally registered location already set (in registry or the config file), then that path will be listed in the first part of the error message. This part of the error message is about the registry key (or config file) itself, so the user can register the install location by setting the registry key (or writing to the config file).
The error now looks like:
A fatal error occurred. The required library libhostfxr.so could not be found.
If this is a self-contained application, that library should exist in [/tmp/console2/bin/Debug/netcoreapp3.0/linux-x64/publish/].
If this is a framework-dependent application, install the runtime in the global location [/usr/share/dotnet] or use the DOTNET_ROOT environment variable to specify the runtime location or register the runtime in [/etc/dotnet/install_location].
In this case the [/usr/share/dotnet] is the currently active global location. So if the /etc/dotnet/install_location exists and points to for example /tmp/dotnet3 then the first part of the error message would contain that path instead (it's the path where we looked for the missing
runtime).
| return true; | ||
| } | ||
|
|
||
| namespace |
There was a problem hiding this comment.
(for my learning) - why the empty namespace?
There was a problem hiding this comment.
It makes the functions in it "private" to this source file. Without the namespace if there would be two source files with the same function name, the linker would complain that there are duplicates. In C# this would be a normal private static member function, but in C++ even private members must be declared in the class declaration which lives in the header file. So changes to privates still affect anybody who includes the header file - even if only causing recompilation. So the empty namespace is a way to have private functions (which don't need access to members) without the necessity to putting them to the header file.
| } | ||
|
|
||
| pal::string_t self_registered_config_location; | ||
| pal::get_dotnet_self_registered_config_location(&self_registered_config_location); |
There was a problem hiding this comment.
Check the return value instead of just the length below?
…on (dotnet/core-setup#6546) With the recent changes apphost now looks for shared framework in 3 potential places: the default global location, the globally registered location and the DOTNET_ROOT path. The error message didn't mention the globally registered location, so add that. Required a bit of refactoring in the pals to avoid code duplication. Commit migrated from dotnet/core-setup@832f825
3 participants
With the recent changes
apphostnow looks for shared framework in 3 potential places: the default global location, the globally registered location and theDOTNET_ROOTpath.The error message didn't mention the globally registered location, so add that.
Required a bit of refactoring in the pals to avoid code duplication.