Skip to content

Use wslview on WSL even when gio is installed#71

Merged
Byron merged 2 commits intoByron:mainfrom
rgwood:fix-wsl-gio-bug
Apr 16, 2023
Merged

Use wslview on WSL even when gio is installed#71
Byron merged 2 commits intoByron:mainfrom
rgwood:fix-wsl-gio-bug

Conversation

@rgwood
Copy link
Copy Markdown
Contributor

@rgwood rgwood commented Apr 15, 2023

This PR fixes a bug I've been experiencing on WSL.

I have gio installed in my WSL environment because it comes bundled with glib. Because of this, open-rs attempts to use gio and fails:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Custom { kind: Other, error: "Launcher \"gio\" \"open\" \"https://google.com\" failed with ExitStatus(unix_wait_status(512))" }', examples/website.rs:4:38
stack backtrace:
   0: rust_begin_unwind
             at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/std/src/panicking.rs:575:5
   1: core::panicking::panic_fmt
             at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/core/src/panicking.rs:65:14
   2: core::result::unwrap_failed
             at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/core/src/result.rs:1791:5
   3: core::result::Result<T,E>::unwrap
             at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/core/src/result.rs:1113:23
   4: website::main
             at ./examples/website.rs:4:5
   5: core::ops::function::FnOnce::call_once
             at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/core/src/ops/function.rs:251:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

To solve this, this PR just reorders the launchers used on unix so that wslview is attempted first. I think this is desirable behaviour because if wslview is present we're almost certainly in WSL.

If you foresee any problems with this approach, I could rewrite this to check whether we're in WSL before attempting to use wslview.

Byron
Byron requested changes Apr 16, 2023
View reviewed changes
Copy link
Copy Markdown
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for bringing this up!

Reordering linux launchers to do better on Windows Subsystem for Linux seems like a bias that would disadvantage the typical linux system which now have to pay for an extra command invocation attempt.

However, an additional file check seems cheap enough and will improve compatibility.

Screenshot 2023-04-16 at 06 42 45

If the above works, could you adjust the PR accordingly or find a cheap way to test for WSL?

Thanks a lot.

@rgwood
Copy link
Copy Markdown
Contributor Author

rgwood commented Apr 16, 2023 •
edited
Loading

Sure thing, I've updated the PR so it will only attempt to run wslview when running in WSL.

I've looked into this space before and I believe the is-wsl crate is the best way to do this check. It's maintained by Sean Larkinn at Microsoft; I have reviewed the crate's code (and contributed to it). The author is actively maintaining it (unlike the older wsl crate). It caches the result with once_cell so we'll only pay the cost once.

I ran cargo bloat before+after this PR and it seems like it adds about 13KB to the open executable on Linux:

image

image

@Byron Byron merged commit 5f1f80f into Byron:main Apr 16, 2023
@Byron
Copy link
Copy Markdown
Owner

Byron commented Apr 16, 2023 •
edited
Loading

Thanks a lot, especially for the due diligence!

I have additionally limited the is-wsl dependency to be only for unix.

And now there is a new release with the fix as well.

@rgwood rgwood mentioned this pull request Apr 17, 2023
Merged
rgwood added a commit to nushell/nushell that referenced this pull request Apr 17, 2023
@rgwood
Upgrade open crate to fix WSL bug (#8905)
4ecec59 
This PR upgrades the [`open`](https://github.com/Byron/open-rs) crate
(used in the `start` command) from 4.0.1 to 4.0.2. This fixes a bug
where `open` doesn't always work properly on WSL:
Byron/open-rs#71
@CosmicHorrorDev CosmicHorrorDev mentioned this pull request Nov 15, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Byron Byron Byron requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rgwood @Byron