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

playwright: package with buildNpmPackage #302759

Merged
merged 2 commits into from
Sep 3, 2024
Merged

Conversation

kalekseev
Copy link
Contributor

@kalekseev kalekseev commented Apr 9, 2024

Description of changes

  • @playwright/test and playwright-core packages built from source
  • python3Packages.playwright: 1.44.0 -> 1.45.1
  • playwright: 1.40.0 -> 1.45.3
  • updated update script
  • renamed playwright-driver into playwright-core to match original name https://www.npmjs.com/package/playwright-core but we can left it as driver to not break things for users
  • removed playwright-test, it's now replaced by playwright https://www.npmjs.com/package/@playwright/test
  • made playwright js package a top level package (python version used previously is just a wrapper around js package)

ref #298944
ref #229475

/nix/store/nxawidis47cx9v15j2smbhnrlzjg498i-playwright-core-1.43.0/cli.js
Usage: npx playwright [options] [command]

Options:
  -V, --version                          output the version number
  -h, --help                             display help for command

Commands:
  open [options] [url]                   open page in browser specified via -b, --browser
  codegen [options] [url]                open page and generate code for user actions
  install [options] [browser...]         ensure browsers necessary for this version of Playwright are installed
  uninstall [options]                    Removes browsers used by this installation of Playwright from the system
                                         (chromium, firefox, webkit, ffmpeg). This does not include branded channels.
  install-deps [options] [browser...]    install dependencies necessary to run browsers (will ask for sudo permissions)
  cr [options] [url]                     open page in Chromium
  ff [options] [url]                     open page in Firefox
  wk [options] [url]                     open page in WebKit
  screenshot [options] <url> <filename>  capture a page screenshot
  pdf [options] <url> <filename>         save page as pdf
  show-trace [options] [trace...]        show trace viewer
  test                                   Run tests with Playwright Test. Available in @playwright/test package.
  show-report                            Show Playwright Test HTML report. Available in @playwright/test package.
  merge-reports                          Merge Playwright Test Blob reports Available in @playwright/test package.
  help [command]                         display help for command

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.

@phaer
Copy link
Member

phaer commented Apr 9, 2024

@kalekseev Nice, thanks for working on this!

The current implementation, using node2nix has the benefit of automatically updating the hashes. Could we do the same here? i.e. update them in updateScript?

@kalekseev kalekseev changed the title [POC] playwright-core: package with buildNpmPackage playwright-test: package with buildNpmPackage Apr 9, 2024
@kalekseev
Copy link
Contributor Author

@kalekseev Nice, thanks for working on this!

The current implementation, using node2nix has the benefit of automatically updating the hashes. Could we do the same here? i.e. update them in updateScript?

we have to write a bunch of bash in update.sh

@phaer
Copy link
Member

phaer commented Apr 9, 2024

@kalekseev Nice, thanks for working on this!
The current implementation, using node2nix has the benefit of automatically updating the hashes. Could we do the same here? i.e. update them in updateScript?

we have to write a bunch of bash in update.sh

Are you going to do so? Otherwise, I could take a look at it next week.
Ideally, version should be kept in sync with the driver via update.sh as well.

@kalekseev
Copy link
Contributor Author

Are you going to do so? Otherwise, I could take a look at it next week. Ideally, version should be kept in sync with the driver via update.sh as well.

@phaer don't know if I will have time to work on this PR this week, feel free to work on it.

BTW After recent changes it looks like playwright, playwright-test, playwright-driver were unified as single package playwright so we should review these options:

  • replace playwright-test and playwright-driver with playwright
  • move python version of playwright out of global packages into pythonPackages and replace it with this package.
  • playwright-python should use ${playwright}/cli.js as driver
  • update.sh should use browser.json from original repository

@kalekseev
Copy link
Contributor Author

@phaer I think I'm done with this one, updated update.sh with chatgpt help, not the best code but good enough for start.

@kalekseev kalekseev marked this pull request as ready for review April 10, 2024 20:58
# Set the correct driver path with the help of a patch in patches
substituteInPlace playwright/_impl/_driver.py \
--replace "@driver@" "${driver}/bin/playwright"
--replace "@driver@" "${driver}/cli.js"
Copy link
Member

Choose a reason for hiding this comment

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

same

@kalekseev kalekseev force-pushed the playwright-core branch 2 times, most recently from 279e7af to 84dce4a Compare June 10, 2024 08:26
@teto
Copy link
Member

teto commented Jun 17, 2024

Result of nixpkgs-review pr 302759 run on x86_64-linux 1

52 packages built:
  • changedetection-io
  • changedetection-io.dist
  • lacus
  • lacus.dist
  • pentestgpt
  • pentestgpt.dist
  • playwright (python311Packages.playwright)
  • playwright-driver
  • playwright-test
  • playwright.dist (python311Packages.playwright.dist)
  • python311Packages.batchspawner
  • python311Packages.batchspawner.dist
  • python311Packages.dockerspawner
  • python311Packages.dockerspawner.dist
  • python311Packages.jupyterhub
  • python311Packages.jupyterhub-ldapauthenticator
  • python311Packages.jupyterhub-ldapauthenticator.dist
  • python311Packages.jupyterhub-systemdspawner
  • python311Packages.jupyterhub-systemdspawner.dist
  • python311Packages.jupyterhub-tmpauthenticator
  • python311Packages.jupyterhub-tmpauthenticator.dist
  • python311Packages.jupyterhub.dist
  • python311Packages.lacuscore
  • python311Packages.lacuscore.dist
  • python311Packages.oauthenticator
  • python311Packages.oauthenticator.dist
  • python311Packages.playwright-stealth
  • python311Packages.playwright-stealth.dist
  • python311Packages.playwrightcapture
  • python311Packages.playwrightcapture.dist
  • python311Packages.pycookiecheat
  • python311Packages.pycookiecheat.dist
  • python311Packages.pytest-playwright
  • python311Packages.pytest-playwright.dist
  • python312Packages.lacuscore
  • python312Packages.lacuscore.dist
  • python312Packages.playwright
  • python312Packages.playwright-stealth
  • python312Packages.playwright-stealth.dist
  • python312Packages.playwright.dist
  • python312Packages.playwrightcapture
  • python312Packages.playwrightcapture.dist
  • python312Packages.pycookiecheat
  • python312Packages.pycookiecheat.dist
  • python312Packages.pytest-playwright
  • python312Packages.pytest-playwright.dist
  • shot-scraper
  • shot-scraper.dist
  • theharvester
  • theharvester.dist
  • urlwatch
  • urlwatch.dist

Copy link
Member

@phaer phaer left a comment

Choose a reason for hiding this comment

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

  • I built this successfully on aarch64-darwin, with a disabled sandbox - but that's already been needed before that PR on darwin.
  • Some smaller style-related comments regarding update.sh. I also think that the update script could maybe be moved to pkgs/development/web/playwright, as it's mostly affecting the driver.nix file there?
  • I think the bundling solution works well, Thanks :)
  • As this PR is more up-to-date now, I'll re-base & update Playwright: browser improvements, update #298944 once this one got merged.

update-source-version python3Packages.playwright "$version"

# Update package-lock.json files for all npm deps that are built in playwright
# TODO: skip if update-source-version reported the same version
Copy link
Member

Choose a reason for hiding this comment

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

That's not checked yet, I believe?

replace_sha() {
sed -i "s|$1 = \".\{44,52\}\"|$1 = \"$2\"|" "$driver_file"
# Function to download `package-lock.json` for a given source path and update hash
update_hash() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
update_hash() {
update_npm_hash() {

or something like this? Just to show that it's about npm/nodejs packages here, we are in ./pkgs/development/python-modules here.

@phaer
Copy link
Member

phaer commented Jul 27, 2024

@kalekseev is there anything one could do to to help pushing it forward? Or is it just waiting to a merge by a commuter.

None of my comments should be blocking anything, this is definitely progress in my view!

@kalekseev
Copy link
Contributor Author

@phaer I believe it's ready to be merged, if someone sees any blockers I'm ready to fix them

@phaer
Copy link
Member

phaer commented Jul 28, 2024

@teto This might be good to merge then, if you don't have any objections? I think you might be the only one with commit bit among the current reviewers :)

@teto
Copy link
Member

teto commented Aug 5, 2024

@phaer yes I am interested in merging this. I want to test it first, most likely next monday. Could you rebase to fix the conflict please ?

@kalekseev kalekseev force-pushed the playwright-core branch 2 times, most recently from b8a8575 to 64de115 Compare August 6, 2024 02:46
@kalekseev
Copy link
Contributor Author

@phaer yes I am interested in merging this. I want to test it first, most likely next monday. Could you rebase to fix the conflict please ?

@teto rebased

@ibrahimsag
Copy link
Contributor

Can we update the playwright to 1.46?

@teto
Copy link
Member

teto commented Aug 19, 2024

@phaer yes I am interested in merging this. I want to test it first, most likely next monday. Could you rebase to fix the conflict please ?

Thanks for rebasing. I am sorry it takes so long. Playwright is something I help with on the side and before merging the PR, I would like to test it on our infrastructure, which needs a patch in staging-next. Once my patch reaches nixos-unstable, I can check this with our playwright code and if all goes well I will merge. So this may take a few more days. Other nixpkgs contributors may merge it before me. I will rebase if need be so you dont spend more time on this.

Copy link
Member

Choose a reason for hiding this comment

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

I couldn't find any removed deps in this commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deps were removed in master, updated commit message

pname = "playwright";
inherit version src;

sourceRoot = "${src.name}"; # update.sh depends on sourceRoot presence
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sourceRoot = "${src.name}"; # update.sh depends on sourceRoot presence
sourceRoot = src.name;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update.sh script depends on this form.

pkgs/development/web/playwright/driver.nix Outdated Show resolved Hide resolved
pkgs/development/web/playwright/driver.nix Outdated Show resolved Hide resolved
pkgs/development/web/playwright/driver.nix Outdated Show resolved Hide resolved
pkgs/development/web/playwright/driver.nix Outdated Show resolved Hide resolved
playwright-driver: 1.40.0 -> 1.46.0
python3Packages.playwright: 1.44.0 -> 1.46.0
@kalekseev
Copy link
Contributor Author

Updated to 1.46.0 and rebased.

@teto
Copy link
Member

teto commented Sep 3, 2024

tested here. Looks good. I think we might want to expose more but this can be done later. Merging.

@teto teto merged commit 23f4d2e into NixOS:master Sep 3, 2024
26 checks passed
@humemm
Copy link

humemm commented Sep 4, 2024

@kalekseev @teto

Just pulled and ran this PR. The build passes fine but the runtime blows up with an error

node:internal/modules/cjs/loader:1148
  throw err;
  ^

Error: Cannot find module '/nix/store/mfiy2g44i2lxmj9rdxns3zhvqdf4cv81-playwright-core-1.46.0/package/cli.js'
    at Module._resolveFilename (node:internal/modules/cjs/loader:1145:15)
    at Module._load (node:internal/modules/cjs/loader:986:27)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:174:12)
    at node:internal/main/run_main_module:28:49 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}

Just reviewed the locations and code and it seems like cli.js is no longer found in /package/cli.js but instead it's jut in the top level directory.

The following is the fix in playwright/default.nix:

# Set the correct driver path with the help of a patch in patches
substituteInPlace playwright/_impl/_driver.py \
  --replace-fail "@node@" "${lib.getExe nodejs}" \
-  --replace-fail "@driver@" "${driver}/package/cli.js"
+  --replace-fail "@driver@" "${driver}/cli.js"

maybe in the future we should add a simple test that runs playwright --version after build.

@kalekseev
Copy link
Contributor Author

maybe in the future we should add a simple test that runs playwright --version after build.

this is python package not playwright itself so playwright --version won't catch this, I'll create PR with fix in a couple of hours

@humemm
Copy link

humemm commented Sep 5, 2024

@kalekseev

maybe in the future we should add a simple test that runs playwright --version after build.

this is python package not playwright itself so playwright --version won't catch this, I'll create PR with fix in a couple of hours

Not sure what you're referring to. The error above was triggered by simply doing playwright --version. This "python" package also exposes the playwright executable/cli when installed.

@phaer
Copy link
Member

phaer commented Sep 5, 2024

@humemm now that this pr is merged, I just need to find time for a rebase of #298944 which includes a NixOS test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants