-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Lookup WSL distros in the registry #10967
Conversation
@check-spelling-bot ReportUnrecognized words, please review:
Previously acknowledged words that are now absentLPCCH lpcs SPACEBAR Unregister xIcon yIconTo accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands... in a clone of the [email protected]:microsoft/terminal.git repository
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉 🗜️ If you see a bunch of garbageIf it relates to a ... well-formed patternSee if there's a pattern that would match it. If not, try writing one and adding it to a Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines. Note that patterns can't match multiline strings. binary-ish stringPlease add a file path to the File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
|
@check-spelling-bot ReportUnrecognized words, please review:
Previously acknowledged words that are now absentLPCCH lpcs SPACEBAR Unregister xIcon yIconTo accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands... in a clone of the [email protected]:microsoft/terminal.git repository
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉 🗜️ If you see a bunch of garbageIf it relates to a ... well-formed patternSee if there's a pattern that would match it. If not, try writing one and adding it to a Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines. Note that patterns can't match multiline strings. binary-ish stringPlease add a file path to the File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
|
{ | ||
std::vector<std::wstring> names{}; | ||
names.reserve(guidStrings.size()); | ||
if (_getWslNames(wslRootKey, guidStrings, names)) |
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.
I believe it would be better to use exception handling as opposed to using booleans.
Notes from looking at the registry keys:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
*valueLengthNeededWithNull = (length / sizeof(wchar_t)); | ||
// If you add one for another trailing null, then there'll actually | ||
// be _two_ trailing nulls in the buffer. | ||
return status == ERROR_MORE_DATA ? ERROR_SUCCESS : status; |
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.
I'm not sure these are HRESULTs
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.
(I am also not certain I understand how this function works/what it is expecting)
This comment has been minimized.
This comment has been minimized.
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
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.
Approve with comments. Unmark automerge so you can see them before it rockets off.
// - <none> | ||
// Return Value: | ||
// - the HKEY if it exists and we can read it, else nullptr | ||
static HKEY openWslRegKey() |
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.
You can't have these return the wil::unique_hkey
straight up?
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.
+1 for returning RAII wrappers.
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.
My preferred API surface when smart wrappers is involved -- and this extends to WRL::ComPtr and all similar variants -- is this:
smart_wrapper_thing GetAThing(); // produce shared wrapper
thing GetAThing(); // produce dumb thing, see note 1
void TakeAThingOptional(thing*); // consume only -- non-owning and non-shared -- optional
void TakeAThing(thing&); // consume only -- non-owning and non-shared -- required
Of course, ref vs pointer is a decision you can make for the specific type. HKEY
is by nature optional.
Putting the smart_wrapper
into the consuming contract forces the caller to have that type (bad) and implies some ownership semantics (dependent on the type).
(1) In general, a function that returns a smart wrapper is subject to the same restrictions as one that returns the thing directly (like HKEY openWslRegKey
): it must ensure that if an exception is encountered before the smart wrapper is populated, it does not leak the resource. Registry functions don't throw, so the need for returning a smart wrapper is reduced. That makes HKEY open...
okay in my book so long as we make sure to cover the error conditions.
// - <none> | ||
// Return Value: | ||
// - the HKEY if it exists and we can read it, else nullptr | ||
static HKEY openWslRegKey() |
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.
+1 for returning RAII wrappers.
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.
I think I have to block because Leonard pointed out a logic error?
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.
LBTM
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.
@zadjii-msft I asked Leonard to block it so I could approve, leaving you with two once you dismiss his. For timezone purposes, it's likely easier on you to dismiss his when you've fixed it 😄
This PR converts the WSL distro generator to use the registry to lookup WSL distros instead of trying to parse the results of `wsl.exe`. `wsl.exe` sometimes takes a very long time to launch the WSL service, which means that on the first launch of the Terminal, WSL distros can sometimes be missing entirely! ## References * Also related is #6160, but I feel that deserves a separate PR for warning when the default profile is a dynamic profile who's source indicated it was gone. ## PR Checklist * [x] Closes #9905 * [x] Closes #7199 * [x] I work here * [ ] Tests added/passed * [ ] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments This is maybe a little BODGY, but hey we get tons of reports of this root cause. ## Validation Steps Performed Ran it locally, it did well. Ran a `wsl --shutdown`, then booted the terminal - seemed to do well. I never was able to repro the slowness myself, but I'd suspect this'll fix it.
🎉 Handy links: |
🎉 Handy links: |
This PR converts the WSL distro generator to use the registry to lookup
WSL distros instead of trying to parse the results of
wsl.exe
.wsl.exe
sometimes takes a very long time to launch the WSL service,which means that on the first launch of the Terminal, WSL distros can
sometimes be missing entirely!
References
warning when the default profile is a dynamic profile who's source
indicated it was gone.
PR Checklist
Detailed Description of the Pull Request / Additional comments
This is maybe a little BODGY, but hey we get tons of reports of this
root cause.
Validation Steps Performed
Ran it locally, it did well. Ran a
wsl --shutdown
, then booted theterminal - seemed to do well. I never was able to repro the slowness
myself, but I'd suspect this'll fix it.