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' instead of 'xdg-open' on Windows #14822

Merged
merged 2 commits into from Feb 27, 2023

Conversation

maxim-belkin
Copy link
Member

@maxim-belkin maxim-belkin commented Feb 27, 2023

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

In WSL, xdg-open is not available -- wslview is. So, added wsl? and wsl_version methods to OS::Linux to conditionally set PATH_OPEN

Additional "require" statements:

  • require "PATH" in utils.rb: needed by PATH.new in which
  • require "utils" in linux.rb: needed by which in os_version
  • require "version" in os.rb: needed by sig { returns(Version) }

To test, execute brew docs

@BrewTestBot
Copy link
Member

Review period will end on 2023-02-28 at 08:36:59 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 27, 2023
@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Feb 27, 2023
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 27, 2023
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

@MikeMcQuaid
Copy link
Member

Makes sense to me, thanks @maxim-belkin! Some brew style failures but happy to land this after those: https://github.com/Homebrew/brew/actions/runs/4280813376/jobs/7453043805

@maxim-belkin
Copy link
Member Author

Thank you, Mike, for approving the pull request!

Library/Homebrew/os/linux.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux.rb Outdated Show resolved Hide resolved
Library/Homebrew/utils.rb Show resolved Hide resolved
@MikeMcQuaid
Copy link
Member

@maxim-belkin some more brew style --fix needed and then should be good to go!

Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>
@MikeMcQuaid MikeMcQuaid merged commit 5eddc77 into Homebrew:master Feb 27, 2023
@MikeMcQuaid
Copy link
Member

Thanks again @maxim-belkin!

@maxim-belkin
Copy link
Member Author

Thanks for the review and feedback, Mike!

@maxim-belkin maxim-belkin deleted the wslview-win branch February 27, 2023 17:55
@RandomDSdevel
Copy link
Contributor

@maxim-belkin, CC @MikeMcQuaid:

     A Linux distribution installed inside Windows Subsystem for Linux v2 isn't guaranteed not to have 'xdg-open' available. Also, custom WSL distributions created manually by a user instead of downloaded from the Microsoft Store aren't guaranteed to have 'wslview' in their 'PATH.' For example, I don't have Homebrew installed on it — I'm perfectly happy with Portage there, though that isn't relevant for a Homebrew issue —, but I use a custom Gentoo WSL installation I made myself and can run graphical applications from inside it using its KDE Plasma installation and X410. Here's what I see in terminal sessions:

… ~ $ which xdg-open
/usr/bin/xdg-open
… ~ $ which wslview
which: no wslview in (~/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/opt/bin:/usr/lib/llvm/15/bin)

This kind of edge case might not be worth handling, at least not yet/at the moment, though?

@maxim-belkin
Copy link
Member Author

Oh, I wasn't aware of such setups. It shouldn't be difficult to check if wslview or xdg-open is available in WSL.

@MikeMcQuaid
Copy link
Member

@maxim-belkin Yeh, a follow-up PR to use which would be nice and just fallback?

@maxim-belkin
Copy link
Member Author

@maxim-belkin Yeh, a follow-up PR to use which would be nice and just fallback?

Sure. We can do something like this (added "echo" in case wslview/xdg-open aren't available):

PATH_OPEN = ((which("wslview") || which("xdg-open")).to_s || "echo").freeze

or like this:

PATH_OPEN = ((OS::Linux.wsl? && k=which("wslview")) ? k.to_s : (k=which("xdg-open")) ? k.to_s : "echo").freeze

or like this:

PATH_OPEN = OS::Linux.wsl? && which("wslview").present? && which("wslview").to_s.freeze
PATH_OPEN ||= which("xdg-open").present? && which("xdg-open").to_s.freeze
PATH_OPEN ||= "echo".freeze

@MikeMcQuaid
Copy link
Member

added "echo" in case wslview/xdg-open aren't available

I don't think that's necessary, no-one has ever mentioned that case.

How about:

PATH_OPEN = if OS::Linux.wsl? && (wslview = which("wslview").presence)
  wslview.to_s.freeze
else
  "xdg-open"
end

to best balance simplicity, current logic, existing logic and avoid handling corner cases no-one has ever reported

@github-actions github-actions bot added the outdated PR was locked due to age label Apr 1, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants