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
Support configuring the browser binary download path #6014
Support configuring the browser binary download path #6014
Conversation
9cee0a9
to
eacce2d
Compare
@lcabral37 thank you for the PR, appreciate it. My worry with this change is that it's becoming a bit complex to understand which download variables do what and how many you may or may not need to set. I'm wondering if there's a nicer way of doing this or we could take an opportunity to tidy up the ENV vars we support in a breaking change release. I'd also be interested to know the situation that means you need to install the binaries outside of @mathiasbynens what do you think? |
Thanks for the quick feedback. Anyhow this is the short story of the pain I've seen with having to download the chromium binaries and I understand that this might not event be the best resolution for many of theses issue (using a mirror host would be better in some of these scenarios for sure) but I honestly think this approach would also be a major gain in this architecture. I've researched a bit through the documentation and could not find any combination that would mach this workflow. And it would make it possible to persist the same configuration for the download path while upgrading puppeteer (it would just download and run the newer revision without additional changes) To answer your question regarding CI systems, I can be sure but our experience is that we use Having bothered you with a 5m read already, I can finish with saying that if you can identify any changes that could make this PR more future prof or at least less complex, I'm willing to make it so. |
@lcabral37 thanks for explaining your situation, that makes sense. I can see the benefit of being able to manage where the binary is put yourselves. @mathiasbynens what do you think to this as a new feature? I think we could rework the ENV vars potentially but at a high level do you think this is a feature worth adding? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Longer-term we'll want to change the default download dir to something like I wonder if we should make this change all at once (to reduce confusion), or land this as-is first, and then change the default later. WDYT? |
I think this can be landed as-is first because it does not block changing the default in the future. |
To clarify, my worry is that people will learn about the new env var, and by the time they start to use it, we'll have changed its meaning again. We could avoid this confusion by ensuring the two changes (this + a future patch that changes the default) make it into the same release, but we'd have to decide soonish. |
By changing the meaning of the env var, do you mean the change of the default value? I think in this case the meaning is still the same: your custom download path would be used. Btw, I am not sure how much effort is it to change the default to something like |
I think 99% of people won't have a strong opinion or care about where Puppeteer downloads things, so I think we can land this and then make a new change in the future and clearly document it as breaking. So I'm happy to ship this as is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little hesitant around default-downloading something to a user's home directory. I think we can ship it as-is, and for me if I don't like where it currently downloads, I could just set the ENV.
I also find this feature quite useful, for example, when I need to switch back and forth my Chromium versions for unit-testing purposes.
I definitely think it makes sense to allow configuring the download path. I see the point in downloading to the home folder to enable sharing binaries across projects or (as here) including the binaries in a CI image, but as a user I would be slightly annoyed if |
When the var is set, that path is used, sure. The difference we'd get when changing the default is when it's not set — the "meaning" changes in the sense that you now need to use the var in different cases. Sounds like there's appetite for not putting large binaries in the home folder by default. Thanks for chiming in, everyone! Let's go with this as-is then. |
@lcabral37 Would you be up for rebasing this patch? |
eacce2d
to
2f49a11
Compare
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
2f49a11
to
267a238
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Most certainly. |
Thanks @lcabral37! I think we can land this soon — I'll just wait for @jschfflr to chime in on the above discussion as well. |
Just my 2 cents: I think, having puppeteer download files to |
@lcabral37 Looks like |
I suggest removing the changes to the |
By adding support for a variable PUPPETEER_DOWNLOAD_PATH it is possible to support downloading the browser binaries into a folder outside the node_modules this will in a way allow to preserve previous downloaded binaries and not required to download then again
267a238
to
60d8869
Compare
@mjzffr if you insist I can remove it, but for the time being I would prefer to leave it there for consistency. Test wise, I agree it should be tested if it is to be there. so I've performed a quick & dirty manual test to validate firefox downloads to the specified folder (if specified) and that it runs form the specified (if specified) @jschfflr It would certainly resolve the problem I have but after thinking for a bit I would dislike this approach as a user as I see that npm modules should be self contained in its ecosystem (node_modules, cache folders, etc ) and not leak out artifacts through the system... Unless I specify otherwise (not trying to defend my PR in this manner), I would find it very annoying to find a Thank you all for your patience |
Some internal breaking changes, but nothing that affects this project afaik. https://github.com/puppeteer/puppeteer/releases/tag/v5.0.0 Main reason for updating is so that 5.3.0 becomes in-range, which introduced support for PUPPETEER_DOWNLOAD_PATH, which can be set to a cached directory (e.g. for CI to save/restore, or to persist locally in a way compatible with npm-ci) Ref puppeteer/puppeteer#6014
* Puppeteer stores the Chromium binary only in node_modules/ (not in $XDG_CACHE_HOME or some other directory that is generally persisted in dev and CI environments, nor in any other directory that one can opt-in to caching/persistence). This was fixed in Puppeteer 5+ (ref puppeteer/puppeteer#6014), through an opt-in PUPPETEER_DOWNLOAD_PATH env var. * I enalbed this in commit 7cd99a2, but it didn't do anything yet as grunt-contrib-qunit wasn't using the newer Puppeteer yet. This was fixed in gruntjs/grunt-contrib-qunit#173.
* Puppeteer stores the Chromium binary only in node_modules/ (not in $XDG_CACHE_HOME or some other directory that is generally persisted in dev and CI environments, nor in any other directory that one can opt-in to caching/persistence). This was fixed in Puppeteer 5+ (ref puppeteer/puppeteer#6014), through an opt-in PUPPETEER_DOWNLOAD_PATH env var. * I enalbed this in commit 7cd99a2, but it didn't do anything yet as grunt-contrib-qunit wasn't using the newer Puppeteer yet. This was fixed in gruntjs/grunt-contrib-qunit#173. * Fix ENV syntax since tildes aren't expanded in this context.
* Puppeteer stores the Chromium binary only in node_modules/ (not in $XDG_CACHE_HOME or some other directory that is generally persisted in dev and CI environments, nor in any other directory that one can opt-in to caching/persistence). This was fixed in Puppeteer 5+ (ref puppeteer/puppeteer#6014), through an opt-in PUPPETEER_DOWNLOAD_PATH env var. * I enalbed this in commit 7cd99a2, but it didn't do anything yet as grunt-contrib-qunit wasn't using the newer Puppeteer yet. This was fixed in gruntjs/grunt-contrib-qunit#173. * Fix ENV syntax (tildes aren't expanded in this context, other variables like $HOME aren't expanded either, and absolute paths like /tmp also can't because Windows runners interpret /tmp as D:\tmp which doesn't exist by default, and Puppeteer requires the parent dir to exist instead of lazy-creating it). Use a local subdirectory instead.
* Puppeteer stores the Chromium binary only in node_modules/ (not in $XDG_CACHE_HOME or some other directory that is generally persisted in dev and CI environments, nor in any other directory that one can opt-in to caching/persistence). This was fixed in Puppeteer 5+ (ref puppeteer/puppeteer#6014), through an opt-in PUPPETEER_DOWNLOAD_PATH env var. * I enalbed this in commit 7cd99a2, but it didn't do anything yet as grunt-contrib-qunit wasn't using the newer Puppeteer yet. This was fixed in gruntjs/grunt-contrib-qunit#173. * Fix ENV syntax. - Tildes aren't expanded in this context. - Other variables like $HOME aren't expanded either. - Absolute paths like /tmp can't be used because Windows runners interpret `/tmp` as `D:\tmp`, which doesn't exist by default, and Puppeteer requires the parent dir to exist instead of simply lazy-creating it. - Can't use a relative directory in the workspace like ".puppeteer" because Puppeteer insists on the variable being an absolute path. - We can use the `github.workspace` builtin variable, which we can subsitute per <https://git.io/JUJEE>.
By adding support for a variable PUPPETEER_DOWNLOAD_PATH it is possible
to support downloading the browser binaries into a folder outside the
node_modules this will in a way allow to preserve previous downloaded
binaries and not required to download then again
This PR is very similar to #5105 but with the addition that the puppeteer runner also makes use of the PUPPETEER_DOWNLOAD_PATH without additional configuration