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

Split out puppeteer's install to be acquired separately to the rest of the modules #2539

Closed
Tyriar opened this issue Nov 4, 2019 · 5 comments · Fixed by #2541
Closed

Split out puppeteer's install to be acquired separately to the rest of the modules #2539

Tyriar opened this issue Nov 4, 2019 · 5 comments · Fixed by #2541
Assignees
Labels
type/automation Relating to CI/CD pipeline, automation, etc.
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Nov 4, 2019

It downloads Chromium which means is busts the GH Actions cache if we try to cache node_modules and also takes a long time to install locally and in CI. We should move it out and only install it when we need the API tests (cd test/api && yarn? "pretest-api": "npm install puppeteer"?).

@Tyriar Tyriar added the type/automation Relating to CI/CD pipeline, automation, etc. label Nov 4, 2019
@Tyriar Tyriar added this to the 4.3.0 milestone Nov 4, 2019
@Tyriar Tyriar self-assigned this Nov 4, 2019
@leomoty
Copy link
Contributor

leomoty commented Nov 5, 2019

How about using a container for testing the API (like we do in devcontainer?).

With PUPPETEER_SKIP_CHROMIUM_DOWNLOAD, you'd skip downloading chromium binaries within node_modules. But iirc, would require to explicitly set the executablePath in puppeteer call.

@Tyriar
Copy link
Member Author

Tyriar commented Nov 5, 2019

Well this is mainly about just speeding up the compile/unit test job, which already gets run in containers. A perk is that it would also lower the startup time for new people. It makes sense to just avoid installing puppeteer unless you need to actually run the API tests, I don't think we should require someone to have docker to run them though.

@leomoty
Copy link
Contributor

leomoty commented Nov 5, 2019

I meant more like PUPPETEER_SKIP_CHROMIUM_DOWNLOAD as an env var would only default for CI. So locally it should still download puppeteer. And could just check the same env var to set explicitly the binary or not.

@Tyriar
Copy link
Member Author

Tyriar commented Nov 5, 2019

👍 seems like the easiest fix: #2541

@leomoty
Copy link
Contributor

leomoty commented Nov 5, 2019

That is what I meant, sorry for "missing the point" first :)

Edit:
I didn't know you wanted to speed up the non-API times

Tyriar added a commit to Tyriar/xterm.js that referenced this issue Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/automation Relating to CI/CD pipeline, automation, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants