playwright: download browsers for darwin #349469
Conversation
@phaer @teto do you have any ideas how to solve the problems? |
| @@ -37,10 +37,15 @@ update_browser() { | |||
| name="$1" | |||
| suffix="$2" | |||
| arm64_suffix="${3:-$2-arm64}" | |||
| if [ "$suffix" = "mac" ]; then | |||
There was a problem hiding this comment.
nit: maybe we can inverse that, i.e. call "update_browser firefox darwin"?
| aarch64-darwin = browsers-mac; | ||
| x86_64-linux = browsers { }; | ||
| aarch64-linux = browsers { }; | ||
| x86_64-darwin = browsers { withWebkit = false; }; |
There was a problem hiding this comment.
Is there something blocking webkit on darwin? If so, we should probably document that at least in a comment or so. Otherwise: lets enable it ;)
There was a problem hiding this comment.
Mac OS has downloads for each os version, eg. mac-14-arm, mac-15-arm etc, I'm not sure from which archive should we provide webkit in nixpkgs?
There was a problem hiding this comment.
enabled using mac-14 build, works on my macos 15 as well
ofborg
bot
added
11.by: package-maintainer
kalekseev
force-pushed
the
playwright-mac-zips
branch
3 times, most recently
from
1b74a13 to
227ecd2
Compare
ofborg
bot
added
10.rebuild-darwin: 11-100
10.rebuild-linux: 11-100
and removed
10.rebuild-darwin: 1-10
10.rebuild-linux: 0
kalekseev
force-pushed
the
playwright-mac-zips
branch
from
227ecd2 to
d63a016
Compare
| arm64_suffix="${3:-$2-arm64}" | ||
| platform="$2" | ||
| stripRoot="false" | ||
| if [ "$platform" = "darwin" ]; then |
There was a problem hiding this comment.
I think we need to handle browsers.webkit.revisionOverrides either here or below ? At least for darwin, but the logic would probably be the same for all platforms.
Otherwise we might regress on older darwin releases compared to the current, impure approach on master (=unsandboxed playwright install).
There was a problem hiding this comment.
I don't see any reason to do this in this pr, it won't affect anything at the moment
There was a problem hiding this comment.
Otherwise we might regress on older darwin releases compared to the current
Can you elaborate on this, I don't quite understand how can we support old macos versions?
Maybe we can leave existing browsers-mac for such setups, but it doesn't work on intel cpu anyway and on aarch it's browser version for macos on CI server I guess.
There was a problem hiding this comment.
Can you elaborate on this, I don't quite understand how can we support old macos versions?
Sure, could have phrased this better! My line of thinking was: The current, impure playwright install probably handles revisionOverrides correctly, i.e. downloads a different binary on older macos releases.
If we don't handle this here, we always default to the binary for the latest macos supported in browser.json, no?
There was a problem hiding this comment.
The current, impure playwright install probably handles revisionOverrides correctly, i.e. downloads a different binary on older macos releases.
Only if nixpkgs binary cache is not used, otherwise browser build for os from nix build machine will be downloaded.
For now revision overrides really used only for webkit on eol macos versions
"webkit": {
"revision": "2083",
"revisionOverrides": {
"mac10.14": "1446",
"mac10.15": "1616",
"mac11": "1816",
"mac11-arm64": "1816",
"mac12": "2009",
"mac12-arm64": "2009"
},
"browserVersion": "18.0"
},
In this pr browser build for macos-14 is used that's why I said that implementing support in the pr won't make any difference. For now I don't even know how to make builds for different macos versions in nix, so supporting overrides for something that can't be built in nixpkgs doesn't make much sense to me.
If you think we should make this deriviation depend on host machine macos version (and know how to implement this) then please let me know, I may consider adding support for this along with revisionOverrides.
| suffix="ubuntu-22.04" | ||
| fi | ||
| fi | ||
| arm64_suffix="$suffix-arm64" |
There was a problem hiding this comment.
nit: might have introduced this myself, not sure - but now that i notice: the variable name could use the same convention as system, so aarch64
| arm64_suffix="$suffix-arm64" | |
| aarch64_suffix="$suffix-arm64" |
| @@ -196,28 +192,7 @@ let | |||
| }; | |||
| }); | |||
|
|
|||
| browsers-mac = stdenv.mkDerivation { | |||
There was a problem hiding this comment.
Love having this removed soon, but I think we should keep the impure builder available behind a flag for one release or so.
The reason is that we don't really have enough real-world testing for users on different darwin setups and we don't want to break working workflows if not needed. I don't think it's needed here and defaulting to your new approach while allowing opt-in to the old one would be perfect :)
Might as well call bin/playwright instead of cli.js
There was a problem hiding this comment.
The reason is that we don't really have enough real-world testing for users on different darwin
we use it on darwin aarch64 so I considered it covered and ready to fix problem reports asap, x86 doesn't work anyway because of #267894
There was a problem hiding this comment.
Thought about this and agree. I think removing this now is reasonable as users could still just call playwright install outside the sandbox, and we don't even set PLAYWRIGHT_BROWSERS_PATH yet.
|
@ofborg test playwright-python |
|
@phaer btw webkit doesn't work on linux in master, I was manage to start it only when I set PLAYWRIGHT_SKIP_VALIDATE_HOST_REQUIREMENTS=1 but after that it fails anyway, probably some dependencies were missed. |
wegank
added
the
2.status: merge conflict
|
This works for me on both, Still think |
kalekseev
force-pushed
the
playwright-mac-zips
branch
2 times, most recently
from
ffa68d5 to
5e94d14
Compare
ofborg
bot
removed
the
2.status: merge conflict
|
Hi y'all! I would very much like to see |
kalekseev
force-pushed
the
playwright-mac-zips
branch
from
9c14328 to
5958ddd
Compare
|
Updated pr with latest playwright version.
@phaer made some progress with this webkit problem, now |
kalekseev
force-pushed
the
playwright-mac-zips
branch
from
5958ddd to
8cad40c
Compare
|
Reverted back to 1.48 because of breaking changes in playwright, the 1.49 changes are in #358638 |
|
|
wegank
added
the
12.approvals: 1
|
looks overall ok. Mind rebasing so I can run a nixpkgs-review on it ? |
kalekseev
force-pushed
the
playwright-mac-zips
branch
from
8cad40c to
697c4c3
Compare
|
@teto rebased |
|
Result of 20 packages failed to build:
62 packages built:
|
wegank
removed
the
12.approvals: 1
|
|
can you comment on the failed nixpkgs-review packages ? are failurs present on master already ? |
wegank
added
12.approvals: 1
@teto jupyterhub is broken in master for independent reasons. But right now all packages that depends on playwright+chromium should fail in master. Playwright uses chromium from nixpkgs and chromium was updated to latest version as a result it doesn't work with playwright because of new headless mode that supported by playwright from version 1.49 Example of the problem this #375005 |
kalekseev
force-pushed
the
playwright-mac-zips
branch
from
697c4c3 to
aeba1dd
Compare
ofborg
bot
removed
the
2.status: merge conflict
|
|
ok let's unblock the next playwright bump |
Things done
Download browsers for Mac OS, fixes #267894
Updated playwright to latest version, fixes #348930
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.