playwright-test: Add PLAYWRIGHT_BROWSERS_PATH to build environment #368391
Conversation
the-sun-will-rise-tomorrow
force-pushed
the
playwright-add-path
branch
from
27dc250 to
ca60ebe
Compare
| @@ -191,9 +192,25 @@ let | |||
| runHook postInstall | |||
| ''; | |||
|
|
|||
| setupHook = writeText "setupHook.sh" '' | |||
| addBrowsersPath () { | |||
| export PLAYWRIGHT_BROWSERS_PATH="${playwright-core.passthru.browsers}" | |||
There was a problem hiding this comment.
Is it possible to override this var? if user wants to run his script with different browsers path
There was a problem hiding this comment.
Sure, you can override it as usual (by setting it in the environment before running the script).
I think overlays etc. should still work too? Or do we need to use pkgs.playwright-driver instead of the local playwright-core definition to pull in customizations? There's a few references to playwright-core already...
There was a problem hiding this comment.
Playwright package named core but in nixpkgs it was named driver initially and we keep it for backward compatibility. Would be nice to migrate to core eventually but at the moment playwright PRs are waiting for months before someone review and merge them so let's use driver
There was a problem hiding this comment.
Sorry, I don't mean the name, but whether we're using the definition from pkgs so that overrides apply. I think that if we want to make packages overridable with things like overlays or overrideAttrs, then we have to use finalAttrs or import ourselves and use that to refer to our package instead of using what we defined directly.
Let me try a few things...
There was a problem hiding this comment.
Actually I don't see any way to override the browser derivation.
I see that in #344614 the browsers were wrapped in makeOverridable, but that was so that a user can customize and use just the browsers (by setting PLAYWRIGHT_BROWSERS_PATH to point to them), not for building a Node or Python package that uses a different set of browsers in its wrapped scripts.
So, it looks like doing it that way was never possible, but we are not making it worse here.
There was a problem hiding this comment.
Is it possible to override this var? if user wants to run his script with different browsers path
Actually forgot about the case if the user sets the variable before running nix-shell (and doesn't use --pure). Changed it so we set it only if it's unset.
Yes, it's still useful, as that creates a
OK, added. |
| @@ -101,6 +112,9 @@ buildPythonPackage rec { | |||
| { | |||
| driver = playwright-driver; | |||
| browsers = playwright-driver.browsers; | |||
| env = runCommand "playwright-env-test" { | |||
There was a problem hiding this comment.
we already have playwright test that uses python package, I guess we can remove this var in it https://github.com/nixos/nixpkgs/blob/26713f779299cd4688ef2f35e7f19cbc2e207f79/nixos/tests/playwright-python.nix#L15 to test the setupHook
There was a problem hiding this comment.
I think this change will not apply to that test. There, we don't run Playwright from a derivation directly, but instead use writePython3Bin which will create a command that we will then run from outside later, with a different environment that won't have that variable.
This allows writing self-contained node scripts (using a nix-shell shebang), without requiring a separate shell.nix which sets PLAYWRIGHT_BROWSERS_PATH to pkgs.playwright-driver.browsers.
…ironment This allows writing self-contained Python scripts (using a nix-shell shebang), without requiring a separate shell.nix which sets PLAYWRIGHT_BROWSERS_PATH to pkgs.playwright-driver.browsers.
the-sun-will-rise-tomorrow
force-pushed
the
playwright-add-path
branch
from
cd300c3 to
a1952a6
Compare
wegank
added
12.approvals: 1
This allows writing self-contained node scripts (using a
nix-shellshebang), without requiring a separateshell.nixwhich setsPLAYWRIGHT_BROWSERS_PATHtopkgs.playwright-driver.browsers.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.