playwright-driver: init at 1.30.1 #223382

teto · 2023-03-27T13:24:10Z

it was actually moved from
pkgs/development/python-modules/playwright/default.nix to its own pkgs/development/web/playwright/driver.nix .

I am trying to package the typescript version of playwright and the browsers are needed there, it's more convenient to split them away from the python module.

Description of changes

Things done

teto · 2023-03-27T14:20:36Z

I wanted to override the driver version to get newer browsers but that was not easy without this.

teto · 2023-04-09T11:52:04Z

I've added some reveiwers so we can get this into nixos-23-05

mguentner · 2023-04-09T23:42:32Z

The split into dedicated packages makes sense to me, however playwright-driver is still not functional regarding firefox and maybe even darwin as a whole as this seems untested 🤷.
Please see my explanation.
Does playwright ff https://nixos.org for you?

There is also #197899 which attempts to fix firefox so that it can be used with playwright.

yrd

Looks good as far as I can tell. Package seems to build as well.

pkgs/development/web/playwright/driver.nix

mguentner · 2023-04-10T11:00:00Z

@yrd Unfortunately building says not much here because of the behavior of the original derivation.
The upstream playwright structure is highly complex: On a FHS linux system, playwright will download (at least) a patched chromium, a patched firefox and a custom webkit browser - all as binaries.
The original derivation masks that by forcefully replacing these binaries with unpatched ones from nixpkgs.
This results in the browsers being unusable by playwright as the necessary features are missing.
Luckily chromium from nixpkgs works for simple tests, but of course the patches from playwright are also missing there, some advanced usage might also break.

yrd · 2023-04-10T12:20:43Z

Yeah… At the time of writing the original derivation patching up a directory structure that matches what Playwright expects worked okay for both browsers, if I'm remembering correctly. I haven't been keeping up with Playwright development recently, so I'm not sure if something fundamental changed upstream that now actually requires their browser patches for regular operation…

mguentner · 2023-04-10T12:33:41Z

@yrd: on which system? NixOS / Linux or darwin?

Does playwright ff https://nixos.org work for you in this shell?

{ pkgs ? import <nixpkgs> {}
}:
pkgs.mkShell {
  name = "playwright-environment";
  buildInputs = [
    pkgs.playwright
  ];
  shellHook = ''
    export PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD="true"
    export PLAYWRIGHT_BROWSERS_PATH="${pkgs.playwright-driver.browsers}"
    export PLAYWRIGHT_BROWSERS_VERSION="${pkgs.playwright-driver.version}"
  '';
}

playwright cr https://nixos.org does work for me

SuperSandro2000 · 2023-04-11T08:52:19Z

pkgs/development/python-modules/playwright/default.nix

+    driver = playwright-driver;
    tests = {
-      inherit driver browsers;
+      driver = playwright-driver;


Suggested change

driver = playwright-driver;

tests = {

inherit driver browsers;

driver = playwright-driver;

inherit driver;

tests = {

inherit driver browsers;

SuperSandro2000 · 2023-04-11T08:53:35Z

pkgs/development/web/playwright/driver.nix

+      sha256 = {
+        x86_64-linux = "0x71b4kb8hlyacixipgfbgjgrbmhckxpbmrs2xk8iis7n5kg7539";
+        aarch64-linux = "125lih7g2gj91k7j196wy5a5746wyfr8idj3ng369yh5wl7lfcfv";
+        x86_64-darwin = "0z2kww4iby1izkwn6z2ai94y87bkjvwak8awdmjm8sgg00pa9l1a";
+        aarch64-darwin = "0qajh4ac5lr1sznb2c471r5c5g2r0dk2pyqz8vhvnbk36r524h1h";
+      }.${system} or throwSystem;


If we are using finalAttrs above, how is the hash overwriting supposed to work here?

as usual with overrideAttrs(...: src = ) ? maybe i dont understand

pkgs/development/web/playwright/driver.nix

SuperSandro2000 · 2023-04-11T08:54:40Z

pkgs/development/web/playwright/driver.nix

+      }.${system} or throwSystem;
+    } ''
+      export PLAYWRIGHT_BROWSERS_PATH=$out
+      ${driver}/bin/playwright install


I doubt that we are cross friendly right now but we should put driver into nativeBuildInputs to be more ready for it.

this is just code moving around, if someone wants to do cross compilation he can do it in a follow up PR

SuperSandro2000 · 2023-04-11T08:55:42Z

pkgs/development/web/playwright/driver.nix

+      jq
+    ];
+  } (''
+    BROWSERS_JSON=${driver}/package/browsers.json


Does this need to be an env? Otherwise we should put it into a nix string

this was just copy/pasted from the current derivation

teto · 2023-04-12T21:24:37Z

so I've adopted most of the recommendations but just for context: this is mostly refactoring, moving code from one file to another.
Why did I do that ? I was asked to help with playwright in nix by colleagues and I found the whole thing confusing. from my understanding the top-level package called playwright is not the "standard" one but the python module, which also packages browsers but not exactly as playwright expects them. My colleagues work with chrome so I haven't tested firefox. Basic tests with chrome worked fine but I stilly worry that this may fail in some cornercase without playwright's custom patches. In which case I will have a look again (and https://github.com/NixOS/nixpkgs/pull/197899/files is interesting in that regard).
To sum up, moving this part of the code out of the python package makes it clearer that it is not tied to the python package (rather the opposite) and it made it easier for me to experience with the original @playwright/test node package (as the version is tied to the node package version too).
I would have liked to update @playwright/test too but it takes 5 or 6 hours regenerating the node package set while node2nix works out of the box on it.

mguentner · 2023-04-17T19:05:52Z

pkgs/development/web/playwright/driver.nix

+      --set SSL_CERT_FILE /etc/ssl/certs/ca-bundle.crt \
+      --set FONTCONFIG_FILE ${fontconfig}
+  '' + lib.optionalString withFirefox ''
+    FIREFOX_REVISION=$(jq -r '.browsers[] | select(.name == "firefox").revision' $BROWSERS_JSON)


Please remove as this is broken (playwright ff https://nixos.org) or integrate this PR #197899 - however it will be easier to review if this is contributed separately.

I saw it was removed on master, I hesitated to remove it because I preferred to have firefox even if faulty but I followed your advice. I've let some (useless) nix code like withChromium as it may be reused again once we fix+reintroduce firefox

Let me know if you approve the PR in which case I'll merge and submit the node "playwright test" package.

Thanks. LGTM!

it was actually moved from pkgs/development/python-modules/playwright/default.nix to its own pkgs/development/web/playwright/driver.nix . I am trying to package the typescript version of playwright and the browsers are needed there, it's more convenient to split them away from the python module. Careful playwright.browsers is not accessible anymore. Apply suggestions from code review Co-authored-by: Yannik Rödel <hey@yannik.info>

yrd · 2023-04-19T09:18:42Z

Awesome, thank you @teto !

github-actions bot added the 6.topic: python label Mar 27, 2023

teto force-pushed the playwright-browsers branch 4 times, most recently from 63fa8ba to b4d5fc3 Compare March 27, 2023 14:07

teto marked this pull request as ready for review March 27, 2023 14:15

teto requested a review from mguentner March 27, 2023 14:21

ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Mar 27, 2023

teto requested review from yrd and techknowlogick April 9, 2023 11:51

yrd approved these changes Apr 10, 2023

View reviewed changes

pkgs/development/web/playwright/driver.nix Outdated Show resolved Hide resolved

pkgs/development/web/playwright/driver.nix Outdated Show resolved Hide resolved

SuperSandro2000 reviewed Apr 11, 2023

View reviewed changes

pkgs/development/web/playwright/driver.nix Outdated Show resolved Hide resolved

SuperSandro2000 reviewed Apr 11, 2023

View reviewed changes

teto force-pushed the playwright-browsers branch from b741b36 to a47d041 Compare April 12, 2023 20:45

ofborg bot requested a review from sephii April 12, 2023 21:39

ofborg bot removed the 10.rebuild-linux: 1 label Apr 12, 2023

teto mentioned this pull request Apr 14, 2023

Package request: playwright test #217693

Closed

teto force-pushed the playwright-browsers branch from a47d041 to 1ae4115 Compare April 17, 2023 09:30

teto changed the title ~~playwright-driver: init at 1.27.1~~ playwright-driver: init at 1.30.1 Apr 17, 2023

mguentner reviewed Apr 17, 2023

View reviewed changes

teto force-pushed the playwright-browsers branch from 1ae4115 to 5d2cc16 Compare April 18, 2023 08:41

teto merged commit 6d9b879 into NixOS:master Apr 18, 2023

teto deleted the playwright-browsers branch April 18, 2023 21:18

teto mentioned this pull request Apr 19, 2023

playwright: fix firefox #197899

Closed

13 tasks

kalekseev mentioned this pull request Jun 4, 2023

playwright: 1.32.1 -> 1.34.3 #235914

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

playwright-driver: init at 1.30.1 #223382

playwright-driver: init at 1.30.1 #223382

teto commented Mar 27, 2023 •

edited

Loading

teto commented Mar 27, 2023

teto commented Apr 9, 2023

mguentner commented Apr 9, 2023

yrd left a comment

mguentner commented Apr 10, 2023

yrd commented Apr 10, 2023

mguentner commented Apr 10, 2023

SuperSandro2000 Apr 11, 2023

teto Apr 12, 2023

SuperSandro2000 Apr 11, 2023

teto Apr 12, 2023

SuperSandro2000 Apr 11, 2023

teto Apr 12, 2023

SuperSandro2000 Apr 11, 2023

teto Apr 12, 2023

teto commented Apr 12, 2023

mguentner Apr 17, 2023

teto Apr 18, 2023

teto Apr 18, 2023

mguentner Apr 18, 2023

yrd commented Apr 19, 2023

playwright-driver: init at 1.30.1 #223382

playwright-driver: init at 1.30.1 #223382

Conversation

teto commented Mar 27, 2023 • edited Loading

Description of changes

Things done

teto commented Mar 27, 2023

teto commented Apr 9, 2023

mguentner commented Apr 9, 2023

yrd left a comment

Choose a reason for hiding this comment

mguentner commented Apr 10, 2023

yrd commented Apr 10, 2023

mguentner commented Apr 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teto commented Apr 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrd commented Apr 19, 2023

teto commented Mar 27, 2023 •

edited

Loading