Skip to content
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

Use wslview on WSL even when gio is installed #71

Merged
merged 2 commits into from
Apr 16, 2023

Conversation

rgwood
Copy link
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.

Copy link
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
Contributor Author

rgwood commented Apr 16, 2023

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
Owner

Byron commented Apr 16, 2023

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 added a commit to nushell/nushell that referenced this pull request Apr 17, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants