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

chore: use playwright to install browsers #30000

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mistercrunch
Copy link
Member

Seems we use playwright to install chromium in some places, but not consistently for chromium and firefox. This makes for confusing/fragile stuff in the Dockerfile. Here I'm just using playwright consistently and in
the docs. Note that this bloats the dev/ci images with BOTH browsers, but people should really just build on top of lean and add their browsers and database drivers from there. We may want to have a single primary browse
r we support and just keep that one in our Dockerfile

@github-actions github-actions bot added the doc Namespace | Anything related to documentation label Aug 23, 2024
@rusackas rusackas requested a review from kgabryje August 23, 2024 16:28
Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

OMG, I love the simplification, and look forward to NOT updating these versions or dealing with GH Issues about them.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM, but I haven't been able to test this, so deferring approval to someone who can confirm that part.

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated
Comment on lines 154 to 151
RUN apt-get update -qq \
&& apt-get install -yqq --no-install-recommends wget bzip2 \
&& wget -q https://github.com/mozilla/geckodriver/releases/download/${GECKODRIVER_VERSION}/geckodriver-${GECKODRIVER_VERSION}-linux64.tar.gz -O - | tar xfz - -C /usr/local/bin \
# Install Firefox
&& wget -q https://download-installer.cdn.mozilla.net/pub/firefox/releases/${FIREFOX_VERSION}/linux-x86_64/en-US/firefox-${FIREFOX_VERSION}.tar.bz2 -O - | tar xfj - -C /opt \
&& ln -s /opt/firefox/firefox /usr/local/bin/firefox \
&& apt-get autoremove -yqq --purge wget bzip2 && rm -rf /var/[log,tmp]/* /tmp/* /var/lib/apt/lists/*
# Cache everything for dev purposes...
RUN playwright install chromium --with-deps
RUN playwright install firefox --with-deps
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious, does with work with multiarch builds? I'm assuming the chromium and firefox binaries are different on x86_64 and arm64. It seems previously we were only installing the x86_64 versions.

Copy link
Member

Choose a reason for hiding this comment

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

@mistercrunch According to the docs, playwright installs the browsers in some local directory, not in usr/local/bin. We can change that with PLAYWRIGHT_BROWSERS_PATH env variable

Copy link
Member Author

Choose a reason for hiding this comment

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

@villebro I think playwright is smart and figures out what platform its own and downloads the right bins.

@kgabryje is targeting usr/local/bin a requirement? seems on this build cypress e2e is working as expected (?) Not sure how to more directly test it than just trusting cypress.

Copy link
Member

Choose a reason for hiding this comment

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

I think cypress installs its own chromium binary. A proper way to test it would be to run a report

@mistercrunch
Copy link
Member Author

I meant to write that I haven't tested this and was looking to comment ask for help or instructions as to how to test this.

@mistercrunch mistercrunch force-pushed the playwright-in-docker branch 2 times, most recently from 6e25c74 to 019cd50 Compare November 8, 2024 21:32
@pull-request-size pull-request-size bot added size/S and removed size/M labels Nov 8, 2024
@github-actions github-actions bot removed the doc Namespace | Anything related to documentation label Nov 8, 2024
@github-actions github-actions bot added the github_actions Pull requests that update GitHub Actions code label Nov 8, 2024
@github-actions github-actions bot removed the github_actions Pull requests that update GitHub Actions code label Nov 8, 2024
@pull-request-size pull-request-size bot added size/M and removed size/S labels Nov 8, 2024
@github-actions github-actions bot added the github_actions Pull requests that update GitHub Actions code label Nov 8, 2024
@github-actions github-actions bot removed the github_actions Pull requests that update GitHub Actions code label Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants