search

dotnet / vscode-dotnet-runtime

VSCode Extension for Installing .NET via VS Code
MIT License
138 stars 256 forks source link

Don't rely on english output for registry lookups #1830

Closed nagilson closed 3 months ago

nagilson commented 3 months ago

⚠️ Problem: We relied on 'ERROR' to determine if an error occurred during a registry lookup. This lookup is to find already existing installs of the .NET SDK on the machine. If the system display language is not english, reg.exe does not return in english. Unfortunately there is no option to standardize output or output language. https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/reg-query

💡 Solution: Read the status code first to check if the query resulted in an error.

❓ Alternative solution: Add registry-js library from GitHub to query registry in a less hacky way. Unfortunately this adds native binaries to our extension and would require us to ship platform specific packages, which is extra work I would prefer not to do.

💥 : Ways this could be improved: The CommandExecutor only returns a string at the moment. It could be improved to return a result object with stdout, stderr, and status code, so we don't need to run the command twice. This is obviously the better solution in the long term. But it would require updating a lot of code, and I don't think it's a priority to do that right now.

♻️ Testing: I was unable to get reg.exe to output in any language besides english but it does seem to happen. Setting the system locale, preferences, display language, and preferred language did not impact reg.exe after the fact. Will rely on vendors to test on a machine with different OS language. image

image

Resolves: https://github.com/dotnet/vscode-dotnet-runtime/issues/1827 https://github.com/dotnet/vscode-csharp/issues/7158

Forgind commented 3 months ago

Is this fixing a theoretical problem or a user-reported problem? I saw you linked two issues, but both included screenshots that said 'error' somewhere, and you said you had difficulty reproducing this for testing. MSBuild looks for Error:, but it isn't a problem because the Error: part isn't localized anyway.

nagilson commented 3 months ago

Is this fixing a theoretical problem or a user-reported problem? I saw you linked two issues, but both included screenshots that said 'error' somewhere, and you said you had difficulty reproducing this for testing. MSBuild looks for Error:, but it isn't a problem because the Error: part isn't localized anyway.

Good question. The German user had 'FEHLER' instead of error and the vendors were able to repro the bug with CN output and also confirm the fix.