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

Plugin: fix Chrome path #451

Merged
merged 8 commits into from Sep 5, 2023
Merged

Plugin: fix Chrome path #451

merged 8 commits into from Sep 5, 2023

Conversation

AgnesToulet
Copy link
Contributor

@AgnesToulet AgnesToulet commented Sep 1, 2023

The Puppeteer update (#433) contained an update to the puppeteer.executablePath function used in plugin mode to compute Chrome executable path.

This PR updates the package scripts to ensure Chrome is always installed under <pluginDir>/chrome so we can use this path in plugin mode.

Fixes #449

scripts/download_chrome.js Fixed Show fixed Hide fixed
scripts/download_chrome.js Fixed Show fixed Hide fixed
scripts/download_chrome.js Fixed Show fixed Hide fixed
scripts/download_chrome.js Fixed Show fixed Hide fixed
scripts/download_chrome.js Fixed Show fixed Hide fixed
scripts/download_chrome.js Outdated Show resolved Hide resolved
const chromeBinDir = out.toString().trim()

console.log(`Moving ${chromeBinDir} content into ${outputPath}/chrome`);
childProcess.execFileSync('mv', [chromeBinDir, `${outputPath}/tmp`]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This command fails when there's no ${outputPath}/tmp directory.

'error': err.message,
'trace': err.stack,
}
console.error(JSON.stringify(errorLog));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This error was actually logged as a "debug" log in plugin mode so that's why it was usually not displayed. This is fixing that and it should help us getting more info from user issues.

@AgnesToulet
Copy link
Contributor Author

@Clarity-89 I found another solution, this should be more portable and I find it much cleaner than the other one. Tested on Windows only, I'll do some tests on Linux.

@Clarity-89
Copy link
Contributor

Clarity-89 commented Sep 4, 2023

@Clarity-89 I found another solution, this should be more portable and I find it much cleaner than the other one. Tested on Windows only, I'll do some tests on Linux.

Tested on Mac and this works! 🎉 I wonder if this going to work in docker, since we're still using Chromium in our image.

@AgnesToulet
Copy link
Contributor Author

I wonder if this going to work in docker, since we're still using Chromium in our image.

This won't impact the Docker image because it only impacts plugin mode.

@Clarity-89
Copy link
Contributor

Ok, yeah the Docker image built locally works for me. I'm asking because according to this issue the renderer also fails in Docker on Linux. Is that a separate issue?

src/app.ts Show resolved Hide resolved
src/app.ts Outdated Show resolved Hide resolved
@AgnesToulet
Copy link
Contributor Author

I'm asking because according to #449 the renderer also fails in Docker on Linux. Is that a separate issue?

Yes it's a bit different but also fixed in this PR. In this custom Docker image, the executable path is provided through the GF_PLUGIN_RENDERING_CHROME_BIN env variable. But as this variable was used after the executable path computation, it also failed on the call to the executablePath() function.
This is why I moved this section:

  if (env['GF_PLUGIN_RENDERING_CHROME_BIN']) {
    config.rendering.chromeBin = env['GF_PLUGIN_RENDERING_CHROME_BIN'];
  }

We shouldn't compute the executable path anymore if it's provided.

Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>
Copy link
Contributor

@Clarity-89 Clarity-89 left a comment

Choose a reason for hiding this comment

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

Great work on this! 🎉

@AgnesToulet AgnesToulet merged commit 364ee01 into master Sep 5, 2023
4 checks passed
@AgnesToulet AgnesToulet deleted the fix-plugin-chrome-path branch September 5, 2023 15:11
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.

Plugin v3.8.0 does not load: "Could not find Chrome (ver. 116.0.5845.96)" from puppeteer
2 participants