Skip to content

playwright: fix firefox #197899

Closed
DGollings wants to merge 1 commit intoNixOS:masterfrom
DGollings:playwright-fix-firefox
Closed

playwright: fix firefox #197899
DGollings wants to merge 1 commit intoNixOS:masterfrom
DGollings:playwright-fix-firefox

Conversation

@DGollings
Copy link
Contributor

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.05 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Oct 26, 2022
@DGollings
Copy link
Contributor Author

DGollings commented Oct 26, 2022

Opened this PR as requested by @SuperSandro2000, this fixes the currently broken firefox binary. But be warned, I have a very basic understanding of Nix, feedback welcome

EDIT: I'm also not entirely sure why it fixes Firefox, I think the Playwright maintainers run some patches in order to enable remote control

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Oct 26, 2022
@xanderio
Copy link
Contributor

Your commit message doesn't fit with Contributing.md. In this case I think it should be playwright: fix firefox

@DGollings DGollings force-pushed the playwright-fix-firefox branch from fa746cb to 7a2a84f Compare October 26, 2022 14:44
@xanderio
Copy link
Contributor

@ofborg build playwright

@DGollings
Copy link
Contributor Author

done @xanderio

@xanderio
Copy link
Contributor

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

@xanderio
Copy link
Contributor

Interesting nixpkgs-review doesn't see any package rebuilds, not sure what is going on.

"https://raw.githubusercontent.com/microsoft/playwright/v${driverVersion}/packages/playwright-core/browsers.json";
sha256 = "11a1n65l43nfyjn0qrsjjyjl7psvqa67k4kiv8x624faga4zl3mk";
};
raw_data = fromJSON (readFile file);
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
raw_data = fromJSON (readFile file);
raw_data = lib.importJSON file;

}:

let
inherit (builtins) fromJSON readFile listToAttrs;
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
inherit (builtins) fromJSON readFile listToAttrs;

Comment on lines +27 to +28
selectSystem = attrs:
attrs.${system} or (throw "Unsupported system: ${system}");
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
selectSystem = attrs:
attrs.${system} or (throw "Unsupported system: ${system}");
selectSystem = attrs: attrs.${system} or (throw "Unsupported system: ${system}");

Comment on lines +89 to +90
url =
"https://raw.githubusercontent.com/microsoft/playwright/v${driverVersion}/packages/playwright-core/browsers.json";
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
url =
"https://raw.githubusercontent.com/microsoft/playwright/v${driverVersion}/packages/playwright-core/browsers.json";
url = "https://raw.githubusercontent.com/microsoft/playwright/v${driverVersion}/packages/playwright-core/browsers.json";

Comment on lines +95 to +100
listToAttrs (map
({ name, revision, ... }: {
inherit name;
value = revision;
})
raw_data.browsers);
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
listToAttrs (map
({ name, revision, ... }: {
inherit name;
value = revision;
})
raw_data.browsers);
listToAttrs (map (
{ name, revision, ... }: {
inherit name;
value = revision;
}
) raw_data.browsers);


# patchelf the binary
wrapper="${firefox-bin}/bin/firefox"
binary="$(readlink -f $(<"$wrapper" grep '^exec ' | grep -o -P '/nix/store/[^"]+' | head -n 1))"
Copy link
Member

Choose a reason for hiding this comment

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

This is at a fixed location? Or can we get that from the firefox attr via passthru?

Comment on lines +170 to +176
interpreter="$(patchelf --print-interpreter "$binary")"
rpath="$(patchelf --print-rpath "$binary")"

find $firefoxoutdir/ -executable -type f | while read i; do
chmod u+w "$i"
[[ $i == *.so ]] || patchelf --set-interpreter "$interpreter" "$i"
patchelf --set-rpath "$rpath" "$i"
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
interpreter="$(patchelf --print-interpreter "$binary")"
rpath="$(patchelf --print-rpath "$binary")"
find $firefoxoutdir/ -executable -type f | while read i; do
chmod u+w "$i"
[[ $i == *.so ]] || patchelf --set-interpreter "$interpreter" "$i"
patchelf --set-rpath "$rpath" "$i"
find $firefoxoutdir/ -executable -type f | while read file; do
chmod u+w "$file"
[[ $file == *.so ]] || patchelf --set-interpreter "$(patchelf --print-interpreter "$binary")" "$file"
patchelf --set-rpath "$(patchelf --print-rpath "$binary")" "$file"

chmod u+w "$i"
[[ $i == *.so ]] || patchelf --set-interpreter "$interpreter" "$i"
patchelf --set-rpath "$rpath" "$i"
chmod u-w "$i"
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
chmod u-w "$i"

fixed by nix


# create the wrapper script
rm $firefoxoutdir/firefox
<"$wrapper" grep -vE '^exec ' > $firefoxoutdir/firefox
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure what this line does

Comment on lines +183 to +184
echo "exec \"$firefoxoutdir/firefox-bin\" \"\$@\"" >> $firefoxoutdir/firefox
chmod a+x $firefoxoutdir/firefox
Copy link
Member

Choose a reason for hiding this comment

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

We can't use wrapProgram?

@teto
Copy link
Member

teto commented Apr 19, 2023

would you mind updating the PR following up on #223382 ? I am willing to help review/get merged

@DGollings
Copy link
Contributor Author

'sure', but its been ~6 months, so I have completely forgotten what this is about :)
Do you want me to PR this into your PR instead?

@teto
Copy link
Member

teto commented Apr 19, 2023

playwright patches browsers to be able to work. Their team makes these patched browsers available on azure. Currently we use nixpkgs' browsers without patches which seems ok for chromium (I was not able to test but I suppose advanced scenarios fail without patches, otherwise what's the point of these patches xD) but from what I read unpatched firefox completely breaks with playwright.
What I am suggesting is you update this PR but after #223382 the content you are interested in has moved quite a bit + firefox support was removed. So might be faster to start from scratch. If you are not interested in this, np let's close ;)

@SuperSandro2000 SuperSandro2000 changed the title fixed firefox playwright: fix firefox Apr 19, 2023
@DGollings
Copy link
Contributor Author

@teto , I'll have a look when I have time, but am quite busy at the moment. Hopefully I'll be able to re-add Firefox support soon

@Janik-Haag Janik-Haag added the 12.first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
@teto
Copy link
Member

teto commented Oct 2, 2024

firefox support was added in #298944 and this is old. Thanks for the effort. Closing

@teto teto closed this Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 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: 0 This PR does not cause any packages to rebuild on Linux. 12.first-time contribution This PR is the author's first one; please be gentle!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants