-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Show dialog when running .NET 6 GUI single-file on 5.0 hostfxr #59929
Conversation
Tagging subscribers to this area: @agocke, @vitek-karas, @VSadov Issue DetailsShould fix #59819
|
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.
LGTM Thanks!!
The innerloop failure is #59908 |
pal::string_t line; | ||
pal::stringstream_t ss(g_buffered_errors); | ||
while (std::getline(ss, line, _X('\n'))) { | ||
if (starts_with(line, _X("Bundle header version compatibility check failed."), true)) |
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.
It is not specific to this particular issue of the GUI not showing anything (and I know we still need to use / check for this string here to handle older versions), but would it be useful to also add more details to the actual error message about the header version - for if/when we have a new bundler version and someone runs against 6.0 hostfxr? My understanding is that for non-GUI, we would just end up with:
Failure processing application bundle.
Bundle header version compatibility check failed.
Maybe something like the major and minor version of the header versus what is valid would at least provide a better hint as to what is wrong?
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.
Good point - we should fix this for 7.0.
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.
But maybe I'll add it to this change (since it affects 7.0 if cooperating with 6.0) 😢
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.
Yeah, I think it would be nice to do for 6.0 - especially since 6.0 is LTS :(
if (starts_with(line, _X("Bundle header version compatibility check failed."), true)) | ||
{ | ||
dialogMsg = pal::string_t(_X("To run this application, you must install .NET Desktop Runtime ")) + _STRINGIFY(COMMON_HOST_PKG_VER) + _X(" (") + get_arch() + _X(").\n\n"); | ||
url = get_download_url(); |
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.
Also include apphost_version=<COMMON_HOST_PKG_VER>
in the URL query?
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.
Oh my - good point!
|
||
const pal::string_t dotnet_folder = get_directory(dotnet_root); | ||
|
||
if (pal::strcmp(dotnet_folder.c_str(), _X("mockhostfxrBundleVersionFailure"))) |
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.
nit: this one uses mockhostfxr
as the prefix for the known folder name and the above uses hostfxr
- can we stick with one or the other?
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.
It should be mockhostfxr ideally
Directory.CreateDirectory(dotnetWithMockHostFxr); | ||
string expectedErrorCode = Constants.ErrorCode.BundleExtractionFailure.ToString("x"); | ||
|
||
var dotnetBuilder = new DotNetBuilder(dotnetWithMockHostFxr, sharedTestState.RepoDirectories.BuiltDotnet, "hostfxrFrameworkMissingFailure") |
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.
Shouldn't the name be mockhostfxrBundleVersionFailure
? Or am I not understanding how these tests are supposed to be set up?
|
||
const pal::string_t dotnet_folder = get_directory(dotnet_root); | ||
|
||
if (pal::strcmp(dotnet_folder.c_str(), _X("mockhostfxrBundleVersionFailure"))) |
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.
It looks like the intent is that the condition should have == 0
. Same for hostfxrFrameworkMissingFailure
above.
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.
:-( - this shows exactly how long I've been away from C++ 😢
We should wait for #59967 before porting to 6.0. We need the apphost_version in the URL. |
Backport to 6.0 should take both this and #59967 together. |
/backport to release/6.0 |
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1308764993 |
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1308764993 |
@terrajobst I believe something is busted w/ my GitHub permissions |
Co-authored-by: Elinor Fung <elfung@microsoft.com> (cherry picked from commit 3d171f9)
…ssing (#60045) * Show dialog when running .NET 6 GUI single-file on 5.0 hostfxr (#59929) Show dialog when running .NET 6 GUI single-file on 5.0 hostfxr (cherry picked from commit 2f76c08) * Fixes based on feedback for the original #59929 (#59967) Co-authored-by: Elinor Fung <elfung@microsoft.com> (cherry picked from commit 3d171f9) Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>
Should fix #59819