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

chromium: add rpath to libGLESv2.so from libANGLE #269345

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

lilyinstarlight
Copy link
Member

Description of changes

Chromium libANGLE-based GL loading was working by accident before, because the cairo lib pulled in libEGL previously (so dlopen didn't need to search rpath when called in libGLESv2) but no longer does and the rpath needs to be added on both the chromium binary and the libGLESv2.so (and yes both even expect to have pciutils available it seems)

Fixes #268490
Fixes #269104
Supersedes and closes #269308

Things done

  • Built on platform(s)
    • x86_64-linux (in progress)
    • 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/)
  • 23.11 Release Notes (or backporting 23.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
  • Fits CONTRIBUTING.md.

Priorities

Add a 👍 reaction to pull requests you find important.

@miangraham
Copy link
Contributor

miangraham commented Nov 23, 2023

-p chromium for a quick test. Builds, runs, plays youtube without issue. Anything specific that needs runtime prodding?

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

2 packages built:
  • chromium
  • chromium.sandbox (chromium.sandbox.sandbox)

@jchv
Copy link
Contributor

jchv commented Nov 23, 2023

So far this seems to work, just got a Chromium build completed. Going to try an Electron app (Element Desktop) because that wasn't fixed for me either by the previous fix I don't think.

Sorta tangential note, is there a good way to detect this kind of condition, a dlopen that succeeds only because the library is already resident? If not, maybe it would be worth cooking up an LD_PRELOAD hook that tries to determine this condition: when dlopen succeeds with RTLD_NOLOAD, but the library searched for can't actually be found in RPATH/LD_LIBRARY_PATH/RUNPATH. That could potentially suss out issues like this earlier.

@CobaltCause
Copy link
Contributor

I got a local copy of this PR and nix run .#element-desktop'd and it seems to be working! Thank you!

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 labels Nov 23, 2023
Copy link
Contributor

@jchv jchv left a comment

Choose a reason for hiding this comment

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

Now after testing both chromium and element-desktop, I can say that this seems to properly solve the problem so far. I'll try testing one of the other chromium-based derivations, but LGTM.

edit: Actually I guess Electron is the only thing that explicitly does chromium.override. So yeah. LGTM!

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 23, 2023
@yu-re-ka yu-re-ka force-pushed the fix/chromium-libangle-hell branch from 9aa576a to 1a78569 Compare November 23, 2023 09:51
@yu-re-ka yu-re-ka merged commit fa094c6 into NixOS:master Nov 23, 2023
Copy link
Contributor

Backport failed for release-23.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-23.11
git worktree add -d .worktree/backport-269345-to-release-23.11 origin/release-23.11
cd .worktree/backport-269345-to-release-23.11
git switch --create backport-269345-to-release-23.11
git cherry-pick -x 1a7856976554fcdc27027c02bfd4b2c626d563be

@yu-re-ka
Copy link
Contributor

Thank you! 💜

Copy link
Contributor

Successfully created backport PR for release-23.11:

@lilyinstarlight lilyinstarlight deleted the fix/chromium-libangle-hell branch November 23, 2023 12:38
@lilyinstarlight
Copy link
Member Author

That could potentially suss out issues like this earlier.

I mean I actually confirmed that was what was happening via ltrace -e dlopen and seeing the returned address from a working chromium version, which basically does similar hooking on dlopen (this was also how I realized the dlopen from the chromium exe was only indirectly what was failing, since libGLESv2.so is the one that couldn't load libEGL.so.1, which is what initially led to my RPATH-not-being-followed confusion)

Sure enough when I compared the ld loads of working and non-working chromium, libEGL was getting pulled in by libcairo early on in the working one but not the non-working one

@lilyinstarlight
Copy link
Member Author

(I also had to end up spending more time than I would like to think about in the ANGLE code and https://source.chromium.org/, to trace back what is going on in libGLESv2.so and what it expects, to confirm that this would be a comprehensive-enough fix...)

@emilylange
Copy link
Member

Thanks a lot!
I haven't had much time over the past few days 🫠

@nonetrix
Copy link
Contributor

nonetrix commented Nov 27, 2023

I seem to still be having this on the Spotify client for some reason, I tested Brave as a another reference and no such issue. Is there going to be a wait until this fix is out for every application? Seems to be refusing to run over 60 FPS on my high refresh rate monitor for this reason as a side effect unfortunately, not a deal breaker but would be nice to see it fixed I guess. However, I acknowledge I had similar issue with Chromium and it not locking to my monitors refresh rate last time I used NixOS months ago, but regardless that seems fixed in Brave (could be updates, or either me switching to AMD in that time and not to mention I am using Wayland now but it's same in XOrg) and the error saying it can't locate OpenGL appears

@yu-re-ka
Copy link
Contributor

spotify is not using our electron packaging, but simply downloading the official snap package and patching it, so this change does not affect (fix or break) it.

@nonetrix
Copy link
Contributor

Strange, well it's broken there it appears. Should I make a issue somewhere or something?

@yu-re-ka
Copy link
Contributor

Yes, you can create an issue, make sure to mention the nixos revision (nixos-version --revision) and ping the spotify maintainers.

@NANASHI0X74
Copy link
Contributor

I'm on nixos-unstable and for me, beeper, element and slack (all electron apps) are still not rendering correctly. I guess their packages have to be updated? 🤔

This PR is in unstable though according to
https://nixpk.gs/pr-tracker.html?pr=269345

Some days ago I tried to update my flake reference to the PR that was superseded by this one, but it took too long for me to build and I didn't want to annoy my coworkers with my fan on 100% for such a long time 😅

@yu-re-ka

This comment was marked as duplicate.

1 similar comment
@yu-re-ka
Copy link
Contributor

slack, beeper => proprietary apps that we extract and use their bundled electron. has nothing to do with this PR, open a respective issue for each app.

Element: what nixpkgs commit are you on exactly?

@NANASHI0X74
Copy link
Contributor

slack, beeper => proprietary apps that we extract and use their bundled electron. has nothing to do with this PR, open a respective issue for each app.

yeah I figured. Although coincidentally they stopped working around the same time I searched nixpkgs issues and found that preceding PR.

let me grab the commit hash...

@NANASHI0X74
Copy link
Contributor

complete `nix flake info` output Resolved URL: git+file:///home/nanashi/Documents/projects/sysconf Locked URL: git+file:///home/nanashi/Documents/projects/sysconf Path: /nix/store/ljg8h42v5d1s9yyqpzqcr54brn965176-source Revision: 23cbf666bacb96c5a58dae91cd7e1e3fd7110b8b-dirty Last modified: 2023-07-31 12:24:34 Inputs: ├───agenix: github:ryantm/agenix/daf42cb35b2dc614d1551e37f96406e4c4a2d3e4 │ ├───darwin: github:lnl7/nix-darwin/87b9d090ad39b25b2400029c64825fc2a8868943 │ │ └───nixpkgs follows input 'agenix/nixpkgs' │ ├───home-manager: github:nix-community/home-manager/32d3e39c491e2f91152c84f8ad8b003420eab0a1 │ │ └───nixpkgs follows input 'agenix/nixpkgs' │ └───nixpkgs follows input 'nixpkgs' ├───devenv: github:cachix/devenv/c8778e3dc30eb9043e218aaa3861d42d4992de77 │ ├───flake-compat: github:edolstra/flake-compat/35bb57c0c8d8b62bbfd284272c928ceb64ddbde9 │ ├───nix: github:domenkozar/nix/7c91803598ffbcfe4a55c44ac6d49b2cf07a527f │ │ ├───lowdown-src: github:kristapsdz/lowdown/d2c2b44ff6c27b936ec27358a2653caaef8f73b8 │ │ ├───nixpkgs follows input 'devenv/nixpkgs' │ │ └───nixpkgs-regression: github:NixOS/nixpkgs/215d4d0fd80ca5163643b03a33fde804a29cc1e2 │ ├───nixpkgs follows input 'nixpkgs' │ └───pre-commit-hooks: github:cachix/pre-commit-hooks.nix/6881eb2ae5d8a3516e34714e7a90d9d95914c4dc │ ├───flake-compat follows input 'devenv/flake-compat' │ ├───flake-utils: github:numtide/flake-utils/5aed5285a952e0b949eb3ba02c12fa4fcfef535f │ ├───gitignore: github:hercules-ci/gitignore.nix/a20de23b925fd8264fd7fad6454652e142fd7f73 │ │ └───nixpkgs follows input 'devenv/pre-commit-hooks/nixpkgs' │ ├───nixpkgs follows input 'devenv/nixpkgs' │ └───nixpkgs-stable: github:NixOS/nixpkgs/9b8e5abb18324c7fe9f07cb100c3cd4a29cda8b8 ├───emacs-overlay: github:nix-community/emacs-overlay/2527e1367b92eea59652378b88108b162484e7d1 │ ├───flake-utils: github:numtide/flake-utils/ff7b65b44d01cf9ba6a71320833626af21126384 │ │ └───systems: github:nix-systems/default/da67096a3b9bf56a91d16901293e51ba5b49a27e │ ├───nixpkgs: github:NixOS/nixpkgs/5a09cb4b393d58f9ed0d9ca1555016a8543c2ac8 │ └───nixpkgs-stable: github:NixOS/nixpkgs/d2e4de209881b38392933fabf303cde3454b0b4c ├───home-manager: github:nix-community/home-manager/db1878f013b52ba5e4034db7c1b63e8d04173a86 │ └───nixpkgs follows input 'nixpkgs' └───nixpkgs: github:NixOS/nixpkgs/5a09cb4b393d58f9ed0d9ca1555016a8543c2ac8

This is the commit: 5a09cb4

@lilyinstarlight
Copy link
Member Author

I'm on nixos-unstable and for me, beeper, element and slack (all electron apps) are still not rendering correctly. I guess their packages have to be updated? 🤔

I know for at least Element it should be fixed now. Can you try deleting ~/.config/*/GPUCache to see if it's just a manifestation of #244742?

@NANASHI0X74
Copy link
Contributor

but it's absolutely certain that beeper and slack are upstream issues and that nothing about our packaging is wrong? Like, is it possible we're missing a runtime library or something?

I must say I only semi-understand how dynamic loaders work and how our packaging of those apps works

@NANASHI0X74
Copy link
Contributor

I know for at least Element it should be fixed now. Can you try deleting ~/.config/*/GPUCache to see if it's just a manifestation of #244742?

Thank you, that indeed fixed all of them 😄

@yu-re-ka
Copy link
Contributor

Ah, right, this issue would not cause incorrect rendering, but just slow software rendering or apps not starting at all.

@NANASHI0X74
Copy link
Contributor

hmm. for me slack and beeper just showed solid black or grey and element had a few lines and red squares drawn

@stereomato
Copy link
Contributor

@NANASHI0X74 got to the folders of those apps in .config and delete any folder called "GPUCache"

@nonetrix
Copy link
Contributor

I'm on nixos-unstable and for me, beeper, element and slack (all electron apps) are still not rendering correctly. I guess their packages have to be updated? 🤔

I know for at least Element it should be fixed now. Can you try deleting ~/.config/*/GPUCache to see if it's just a manifestation of #244742?

Didn't fix it in my case

@yu-re-ka
Copy link
Contributor

yu-re-ka commented Nov 27, 2023 via email

@nonetrix
Copy link
Contributor

The issue with Spotify appears to be fixed on my system now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
10 participants