Attempt to use explorer.exe on http|https

[spenserblack]

Part of my motivation of this PR is to start a conversation on this.

After noticing that wslview felt really slow, I looked into the source code to see what the cause was. I noticed that wslview was doing a lot of stuff which, IMO, is unnecessary for the purposes of gh.

What wslview does is:

• Queries the Windows Registry via reg.exe to check if the build number is high enough (not used when opening URLs)
• Validates the URL by curling it and asserting that content was returned (a potential source of slowness, this can be skipped with --skip-validation-check on newer versions of wslview).

And, after these validations, what it boils down to is calling either /mnt/c/Windows/System32/WindowsPowerShell/v1.0/powershell.exe -NoProfile -NonInteractive -ExecutionPolicy Bypass -Command "explorer.exe $URL" or /mnt/c/Windows/System32/cmd.exe /c explorer.exe "$URL". Starting a PowerShell process seems to contribute a lot to the slowness.

There's a bit of extra work in case a different prefix is used than /mnt or Windows is installed to a different drive than C:, but this should sum up the vast majority of cases.

I think these validations aren't necessary as long as LookPath is used, and explorer.exe can be called directly. I've noticed a pretty decent gain in speed when using GH_BROWSER=explorer.exe over GH_BROWSER=wslview.

[williammartin]

Very interesting investigation, thanks! Can't say I know too much about wslview usage or what the pros and cons might be here.

Would it make sense to add explorer.exe before wslview? In that way, if there are some systems where explorer.exe can't be found, we don't cause a regression?

[spenserblack]

if there are some systems where explorer.exe can't be found, we don't cause a regression?

🤔 Based on my reading of the source code, wslview won't be able to do much unless explorer.exe exists on the host system.
https://github.com/wslutilities/wslu/blob/180e5fd66571f8f40525eca7172222debe6934d0/src/wslview.sh#L124
And using explorer.exe from WSL is even documented as an example for accessing the current directory, so it's safe to assume that explorer.exe is available in WSL IMO.

I can think of 2 reasons to fall back to wslview:

1. If the user is using WSL, explorer.exe should exist in almost all cases, since it's a standard Windows executable. But I suppose it could be possible that explorer.exe isn't discoverable on the subsystem, but cmd.exe/powershell.exe is, like if the user removed /mnt/c/Windows/ from $PATH. In that case wslview would work when running explorer.exe directly wouldn't.
2. If, for whatever reason, the user has wslview installed when running Linux as the main OS, not in a subsystem, then it's wslview's responsibility to say "whoah, this isn't WSL, did you mean to use this?"

If those reasons make sense to you, I'll add wslview as a fallback as requested 🙂

[williammartin]

I think based on your investigation and usage of WSL you're in a much better position to make a determination here. I guess I'm just concerned that with such little experience in WSL, I want to derisk this. However, I think it would be fine to accept the PR as-is and use it as an opportunity to discover unknowns. The cost of breaking people here is not incredibly high, as there are a variety of workarounds (e.g. GH_BROWSER) until we would ship a fix.

What do you think? Learning opportunity?

[spenserblack]

🤔 Coming back to this with a fresh mind, I think there's no harm in falling back to wslview if explorer.exe is unavailable, so I think it's reasonable to do. I'll add back wslview as a fallback.

I feel like most unknowns can also be discovered by keeping wslview, and those unknowns will likely be raised in a prettier format 😆

[spenserblack]

Oh, I didn't even realize until just now taking a closer look at the source code, but this project also supports the file:// protocol (I was focused only on HTTP(S) requests). Using explorer.exe will definitely break that. So maybe using explorer.exe is more of a "hack" to gain speed, and it should be up to the user rather than something officially supported.

So now I'm actually leaning heavily towards leaving everything as it is and closing this PR. What do you think?

Still not a fan of wslview slowing things down by starting up a powershell.exe process, and duplicating requests by curling the URL in the validity check, but that could perhaps be addressed in wslview itself.

[williammartin]

Hmm, I wonder if that's support that just came in with the fork from pkg/browser. I can't think of any place in which gh would open something with a file protocol in the browser.

However, it's likely there are some other modules depending on this one, even if we don't officially support it. Maybe we could look at the url scheme and only add explorer.exe if it's not file://.

[spenserblack]

However, it's likely there are some other modules depending on this one, even if we don't officially support it. Maybe we could look at the url scheme and only add explorer.exe if it's not file://.

Works for me! Since this is a bit more complex than a 1-line change and I'd like to add a test for this, I'll do this later when I get to my main device.

[williammartin]

Thanks. I created cli/cli#10466 to track getting this into gh as well.

[spenserblack]

Drafting until I address cli/cli#10466 (comment) and have a chance to do a reality check on a device with WSL

[williammartin]

Closing on account of discussion in cli/cli#10466