Fix MacOS Catalina Regex issue #149
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Mac OS Catalina appends a hex code to the end of the path output by LSRegisters, the regex that finds all the chrome executables looked for a newline immediately following the end of the .app. Now the regex finds the paths again, and hex code is trimmed off before being tested for file access and added to the array
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
| @@ -33,13 +33,13 @@ export function darwin() { | |||
|
|
|||
| execSync( | |||
| `${LSREGISTER} -dump` + | |||
| ' | grep -i \'google chrome\\( canary\\)\\?.app$\'' + | |||
| ' | grep -i \'google chrome\\( canary\\)\\?.app.*$\'' + | |||
There was a problem hiding this comment.
that's a bit aggressive (it matches ~15 lines locally for me)
is there any narrower search we could put on it? a particular length limit perhaps or additional filtering on the JS side?
There was a problem hiding this comment.
I added a space between the .app and the wildcard to prevent the regex from returning subdirectories, that took me from 4 results down to 2. One of them is my actual correct chrome install and the other is a copy of chrome in my trash. I could limit the regex to the applications folder without too much trouble, but that could break the command for anyone who has it installed somewhere else. I'm not that great with regular expressions so I may need some help removing the "trashed" version
There was a problem hiding this comment.
@ajoy39 Just an FYI, .*$ is redundant in most cases, because it matches everything (including nothing) up to the end of the string. It would have been better to remove $ altogether and have /google chrome(canary )?\.app/i make no assumptions about what characters follow it.
I amended this as part of #197, and felt I should let you know. :-)
There was a problem hiding this comment.
@Alhadis oh, thanks for letting me know! I’m not great with regex so any help is appreciated!
src/chrome-finder.ts
Outdated
| @@ -33,13 +33,13 @@ export function darwin() { | |||
|
|
|||
| execSync( | |||
| `${LSREGISTER} -dump` + | |||
| ' | grep -i \'google chrome\\( canary\\)\\?.app$\'' + | |||
| ' | grep -i \'google chrome\\( canary\\)\\?.app .*$\'' + | |||
There was a problem hiding this comment.
now this matches nothing on pre-catalina versions like mine :)
how about let's just remove the $ business completely (leave as ?.app') and do the rest of our filtering in JS where it's a little easier to reason about?
There was a problem hiding this comment.
You are absolutely right, I didn't check that one on the Mojave mac mini before I pushed it up. I reverted that change, and put a check to pushing to the installations array to make sure items are unique. Between that and the canAccess() function that will ensure that only valid, unique chrome apps are found and added to the array.
There was a problem hiding this comment.
this LGTM, but I'll probably want to follow this up with better testing in this file anyhow :)
would you mind donating the result of /System/Library/Frameworks/CoreServices.framework/Versions/A/Frameworks/LaunchServices.framework/Versions/A/Support/lsregister -dump in a text file for us or at least the section that has chrome versions to help us out with that? :)
|
Sure thing, here's the chrome related stuff including entries for Chrome Helper and the Chrome.app that was in my trash. |
Mac OS Catalina appends a hex code to the end of the path output by LSRegisters, the regex that finds all the chrome executables looked for a newline immediately following the end of the .app.
Now the regex finds the paths again, and hex code is trimmed off before being tested for file access and added to the array
Fixes #146