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.

Sign up for GitHub

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

Search in Path for chrome.exe #218

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Search in Path for chrome.exe #218

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8219658
Search in Path for chrome.exe
magneticflux- Oct 31, 2020
e2edaf5
Merge branch 'master' into patch-1
magneticflux- Mar 15, 2022
5b9a17e
Incorporate `PATH` search into prefix and suffix list
magneticflux- Mar 15, 2022
0324b89
Use `which -a` on Linux and `where` on Windows to find more binaries
magneticflux- Mar 15, 2022
9872600
Merge branch 'main' into patch-1
brendankenny May 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/chrome-finder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,12 +197,22 @@ export function win32() {
installations.push(customChromePath);
}

// Search common locations for chrome.exe
prefixes.forEach(prefix => suffixes.forEach(suffix => {
const chromePath = path.join(prefix, suffix);
if (canAccess(chromePath)) {
installations.push(chromePath);
}
}));

// Search %PATH% for chrome.exe
process.env.Path?.split(';')?.forEach(prefix => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • shouldn't this be PATH? Did it work for you as Path? Or are env vars in windows cased like that?
  • Seems we could search PATH in mac (darwin in our code) and linux too
  • I think the prefixes array about could have this added (...process.env.Path.split(';')), and this extra loop can be removed

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

On Linux, which is used, so manual PATH parsing is unneeded.

On Darwin, lsregister is used, which I think means it looks up apps:

The Launch Service database controls the Finder's 'Open With' right click, contextual menu.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We ought to be able to use which for mac too. Oh, and there is which -a to list all the commands on PATH (in the expected order, evaluating PATH left to right such that the first command printed is what the shell would use). There's the question of which should take precedence here: the lsregister result or the PATH result. I'd suggest the path result (although this is probably rare to have chrome on your PATH, I don't even have that and I have multiple chromes installed)

The windows equivalent is where (which by default prints everything matching the PATH), although I believe way you're doing it in the current PR is equivalent so not worth using it (but! who know, maybe there is some strange relic of windows behavior we couldn't possible know about).

What do you think of using where and which -a directly to avoid manual PATH processing? At the least, it may be simpler. very curious if it could be more correct.

Copy link
Collaborator

@connorjclark connorjclark Mar 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

oh, found a reason to use where over doing it manually: in windows there's a way to use semicolons for individual items in the PATH... https://superuser.com/questions/584870/how-can-i-add-a-folder-containing-a-semicolon-to-the-windows-path . so if we use where we don't have to worry about the particulars of parsing PATH at all, which apparently isn't as trivial as unix (in unix/POSIX it is impossible to escape the semicolon)

Feel free to leave the PR as-is if you like, and I can work on a follow-up. I do appreciate the quick response after over a year of inactivity :)

Copy link
Author

@magneticflux- magneticflux- Mar 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think using which for macOS is a good idea for this PR since applications are installed differently (and in different formats) and I don't have a machine to test it out.

I implemented which -a without much trouble.

I implemented using where and it detects the binaries properly, but I ran into another issue with Scoop: the default chrome.exe shim that Scoop makes sets --user-data-dir and due to what I perceive as a Chrome bug, the later flags appended by chrome-launcher do not override it.

const chromePath = path.join(prefix, 'chrome.exe')
if (canAccess(chromePath)) {
installations.push(chromePath)
}
});

return installations;
}

Expand Down