Skip to content

Playwright: browser improvements, update#298944

Merged
teto merged 2 commits intoNixOS:masterfrom
phaer:playwright
Sep 21, 2024
Merged

Playwright: browser improvements, update#298944
teto merged 2 commits intoNixOS:masterfrom
phaer:playwright

Conversation

@phaer
Copy link
Member

@phaer phaer commented Mar 25, 2024

Description of changes

  • Support Firefox & Webkit tests on NixOS (and Ubuntu, i.e. in github actions) - previously only Chromium was supported.
  • Add data from browsers.json to nixpkgs via update.sh
    in order to use patched upstream browsers without IFD
  • Use autoPatchElfHook to make those browsers run on nixos
  • Reuses attrs from pkgs.firefox-bin for firefox
  • browser hashes are updated via update.sh
  • theres a new nixos vm test for all 3 supported browsers.

TODO long term

  • Consider using the same approach on darwin, where browser builds are currently impure (using playwright install)
  • check playwrights browsers patches and consider using nixpkgs browsers to build from source, instead of pulling binaries for firefox, webkit and ffmpeg

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Mar 25, 2024
@ofborg ofborg bot requested review from techknowlogick and yrd March 25, 2024 15:25
@ofborg ofborg bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Mar 25, 2024
@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Apr 2, 2024
@phaer phaer marked this pull request as ready for review April 2, 2024 11:42
@phaer phaer requested a review from teto April 2, 2024 11:42
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. label Apr 2, 2024
@phaer
Copy link
Member Author

phaer commented Apr 2, 2024

EDIT: Nevermind, I messed up. Going to push a fix right after local tests pass.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/running-playwright-tests/25655/28

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1632

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 22, 2024
@wmertens
Copy link
Contributor

LGTM - any reason why this isn't merged? (apart from the conflict)

@phaer phaer force-pushed the playwright branch 2 times, most recently from 2c61f6f to 231ccc0 Compare May 25, 2024 11:55
@phaer
Copy link
Member Author

phaer commented May 25, 2024

@wmertens None that I am aware of. Just resolved the merge conflict which was due the recent python reformating PR.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label May 25, 2024
@gungun974
Copy link
Contributor

Just a question like that, but I see in that PR you update @playwright/test to version 1.42.1.

Should not update to 1.44.1 instead or it's okay you think ?

@phaer
Copy link
Member Author

phaer commented May 25, 2024

Yes, this PR is 2 months old already. Could upgrade if needed, but as written in the resolved thread in #298944 (comment) I'd prefer to do so in #302759 in to keep churn to a minimum:)

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

Please cleanup the commit history

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 6, 2024
tembleking added a commit to figarocorso/python-bizneo that referenced this pull request Jul 19, 2024
…the latest version

This should be reverted once NixOS/nixpkgs#298944 is merged.
@teto
Copy link
Member

teto commented Sep 16, 2024

the test times out locally too (x86)

@phaer
Copy link
Member Author

phaer commented Sep 17, 2024

the test times out locally too (x86)

Hm...I wonder what the issue is here. It just ran successfully for me on two different x86_64-linux nixos machines; one of them not managed by to check whether it's some obscure setting or so ;) It also succeeded on on aarch64-linux in ofborg. Going to ask ofborg to run again to see whether it's flaky?

EDIT: It's either flaky or i misunderstand how the checks are run in CI; can re-produce after a rebase.

@teto At this point I am guessing, but could you check whether adding --disable-gpu to chromiums flags in the test changes something for you?

Just wanted to use firefox with playwright half a year ago, wrote a test and now I am trying to get the test to pass with chromium 😅

@phaer
Copy link
Member Author

phaer commented Sep 17, 2024

@ofborg test playwright-python

@phaer
Copy link
Member Author

phaer commented Sep 17, 2024

Rebasing on top of the new playwright release in master and using --disable-gpu seem to have fixed the test, @teto?

The webkit minibrowser is broken inside the test, but seems to work outside of it. I excluded it from the test for now, as my priority was to get firefox working and a nixos vm test for the browsers to begin with :)

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

looks fine to me otherwise, didn't test

Copy link
Member

Choose a reason for hiding this comment

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

We could make this unconditional but whatever. Fine by me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it won't hurt to be explicit about supported platforms. :)

@phaer
Copy link
Member Author

phaer commented Sep 20, 2024

Applied review suggestions; hope this is finally good to merge 🙏 😉


short rant about late-stage style reviews and workflows here:

I really long for a future where we either have a linter (in CI and locally) for things like sha256->hash when its deemed important enough.
And/or a platform which makes it easier to review rebased commits efficiently. The problem with such reviews late in the cycle is that it technically invalidates prior reviews (and produces additional overhead for a pull, fix, rebase, push cycle compared to a linter).

I also believe we could have done those changes in the next PR (pure macOS browsers are planned and I am interested in browsers built from source, but let's see how long my energy lasts for this 😅 )


the innocent change from sha256 -> hash caused:

  • ofborg failing to eval because i forgot to update the hashes
  • me running update.sh to check if it works for the full pipeline
  • seeing no git diff
  • discovering that nix-prefetch-url seemingly doesn't support a format fitting hash
  • rewriting it to use update.sh `nix store prefetch-file --json --hash-type sha256 --unpack "$1" | jq -r .hash
  • waiting for the downloads
  • rebasing to squash the commits

and all that for no functional difference while there's about 56k instances of sha256 usage
in master according to rg. 😕

via update.sh. This lets us support playwright with browsers
other than chromium on linux. Building them from source would
be one step further.
For some reason, chromium, which is still the nixpkgs version hangs
 inside the normal test vm, while working fine in .driverInteractive.

I suspect that might have to do with the existence of a display in
.driverInteractive. Neither vm does run X11 or wayland.

# See here for the Chrome options:
# https://github.com/NixOS/nixpkgs/issues/136207#issuecomment-908637738
# We add --disable-gpu to be able to run in gpu-less environments such
Copy link
Member

Choose a reason for hiding this comment

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

this comment is outdated but let's remove it in future PRs.

@teto
Copy link
Member

teto commented Sep 21, 2024

the test still times out hre for some reason but let's not penalize contributors for adding tests. Considering the effort here, I assume you are in for the long run :)

@teto teto merged commit a6df665 into NixOS:master Sep 21, 2024
@phaer phaer deleted the playwright branch September 21, 2024 21:30
@altano
Copy link

altano commented Sep 21, 2024

🎉🙏

@teto teto mentioned this pull request Oct 2, 2024
13 tasks
bitbloxhub added a commit to bitbloxhub/playwright-web-flake that referenced this pull request Dec 14, 2025
Ports the changes from NixOS/nixpkgs#298944 and
other subsequent fixes to this flake.

Fixes pietdevries94#7
bitbloxhub added a commit to bitbloxhub/playwright-web-flake that referenced this pull request Dec 15, 2025
Ports the changes from NixOS/nixpkgs#298944 and
other subsequent fixes to this flake.

Fixes pietdevries94#7
bitbloxhub added a commit to bitbloxhub/playwright-web-flake that referenced this pull request Dec 15, 2025
Ports the changes from NixOS/nixpkgs#298944 and
other subsequent fixes to this flake.

Fixes pietdevries94#7
bitbloxhub added a commit to bitbloxhub/playwright-web-flake that referenced this pull request Dec 25, 2025
Ports the changes from NixOS/nixpkgs#298944 and
other subsequent fixes to this flake.

Fixes pietdevries94#7
bitbloxhub added a commit to bitbloxhub/playwright-web-flake that referenced this pull request Jan 8, 2026
Ports the changes from NixOS/nixpkgs#298944 and
other subsequent fixes to this flake.

Fixes pietdevries94#7
bitbloxhub added a commit to bitbloxhub/playwright-web-flake that referenced this pull request Mar 9, 2026
Ports the changes from NixOS/nixpkgs#298944 and
other subsequent fixes to this flake.

Fixes pietdevries94#7
bitbloxhub added a commit to bitbloxhub/playwright-web-flake that referenced this pull request Mar 9, 2026
Ports the changes from NixOS/nixpkgs#298944 and
other subsequent fixes to this flake.

Fixes pietdevries94#7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants