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

GOOGLE_CHROME_SHIM now responds to --version and --product-version. #73

Merged
merged 1 commit into from
Jun 21, 2019
Merged

GOOGLE_CHROME_SHIM now responds to --version and --product-version. #73

merged 1 commit into from
Jun 21, 2019

Conversation

kapoorlakshya
Copy link
Contributor

@kapoorlakshya kapoorlakshya commented Jun 14, 2019

This change adds out-of-the-box compatibility with the webdrivers gem, which apparently quite a few Heroku users are using. Currently, the users have to use a workaround in their project to get the Chrome version from $GOOGLE_CHROME_BIN, so that webdrivers can download the appropriate version of chromedriver, and then switch back to $GOOGLE_CHROME_SHIM to launch Chrome.

See titusfortner/webdrivers#40, titusfortner/webdrivers#72, and titusfortner/webdrivers#134.

This simple change will provide the convenience of not needing any hacks to make the two work :).

@twalpole
Copy link

@kapoorlakshya That's great - hopefully it's accepted -- I do wonder why chrome on linux has --product-version and --version while on MacOS there's only --version. Since on linux chrome does accept both --version and --product-version (one is just the number the other has extra text) It might be nice to have the shim do the right thing with either of them.

@kapoorlakshya
Copy link
Contributor Author

It might be nice to have the shim do the right thing with either of them.

I agree. I'll go ahead and add another condition to have it respond to --version as well.

@kapoorlakshya kapoorlakshya changed the title Make GOOGLE_CHROME_SHIM respond to --product-version. GOOGLE_CHROME_SHIM now responds to --version and --product-version. Jun 15, 2019
@kapoorlakshya
Copy link
Contributor Author

$ google-chrome-stable --version
Google Chrome 74.0.3729.108

$ google-chrome-stable --product-version
74.0.3729.108

@kapoorlakshya
Copy link
Contributor Author

kapoorlakshya commented Jun 20, 2019

Hi @joshwlewis and @jabrown85, is there any chance of this getting reviewed/merged?

@richardvenneman
Copy link

Can confirm this fix works great in production, thanks 🙏

@jabrown85 jabrown85 merged commit f77e60c into heroku:master Jun 21, 2019
@jabrown85
Copy link
Contributor

I deployed this to the buildpack registry and it's in master for those not using the shorthand buildpack name heroku/google-chrome. Thanks for the contribution!

@kapoorlakshya
Copy link
Contributor Author

@jabrown85 Thank you very much!

@@ -171,7 +171,13 @@ BIN_DIR=$BUILD_DIR/.apt/usr/bin
rm $BIN_DIR/$SHIM
cat <<EOF >$BIN_DIR/$SHIM
#!/usr/bin/env bash
exec \$HOME/.apt/opt/google/$BIN --headless --no-sandbox --disable-gpu --remote-debugging-port=9222 \$@
if [ $1 = "--version" ]; then
Copy link

Choose a reason for hiding this comment

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

Shouldn't the $ in $1 be escaped ?
When using the heroku/google-chrome buildpack and running heroku ci:debug, I get

~ $ cat $GOOGLE_CHROME_SHIM   
#!/usr/bin/env bash
if [ /app = "--version" ]; then
  exec $HOME/.apt/opt/google/chrome/chrome --version
elif [ /app = "--product-version" ]; then
  exec $HOME/.apt/opt/google/chrome/chrome --product-version
else
  exec $HOME/.apt/opt/google/chrome/chrome --headless --no-sandbox --disable-gpu --remote-debugging-port=9222 $@
fi
~ $ 

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes this is correct, I'm seeing this:

if [ /tmp/build_9b8b699572a1f61d9096ed0e275f2c41 = "--version" ]; then

because the $1 isn't escaped.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in #74

@kapoorlakshya
Copy link
Contributor Author

Thanks for fixing it, but just curious why you would call cat on the shim. I thought the shim was solely used to launch Chrome with the extra args, like headless.

Sorry, not a Heroku user.

@isc
Copy link

isc commented Jul 9, 2019

@kapoorlakshya I called cat on the shim while debugging to confirm that I still needed the workaround despite what this PR said.

@kapoorlakshya
Copy link
Contributor Author

@isc Can you please share what's not working? I would appreciate a bug report on the webdrivers repo when you have a chance.

@kapoorlakshya kapoorlakshya deleted the make_shim_respond_to_version branch July 9, 2019 18:22
@kwlockwo
Copy link
Contributor

kwlockwo commented Jul 10, 2019

@kapoorlakshya it is fixed in #74

@isc was just doing the cat to illustrate the problem. Which was that $1 was being substituted during the build which from memory is the root directory where your app is built (often set as ${BUILD_DIR} in buildpacks).

This has been merged to master and pushed to our buildpack registry.

@kapoorlakshya
Copy link
Contributor Author

@kwlockwo Right. I was addressing his comment which sounded like the workaround for webdrivers is still required, but perhaps I misunderstood.

Anyway, thanks again for #74.

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.

6 participants