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

kitty: fix loading of library #74874

Open
wants to merge 2 commits into
base: master
from

Conversation

@Luflosi
Copy link
Contributor

Luflosi commented Dec 3, 2019

When trying to play a sound, kitty prints an error message because it cannot find libcanberra.so:

Failed to load libcanberra.so, cannot play beep sound, with error: libcanberra.so.0.2.5: cannot open shared object file: No such file or directory

This is fixed by patching the path to the library.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @tex @rvolosatovs @Ma27

When trying to play a sound, kitty prints an error message because it cannot find `libcanberra.so`:
```
Failed to load libcanberra.so, cannot play beep sound, with error: libcanberra.so.0.2.5: cannot open shared object file: No such file or directory
```
This is fixed by patching the path to the library.
@Luflosi

This comment has been minimized.

Copy link
Contributor Author

Luflosi commented Dec 3, 2019

BTW, is there a good way to do this upstream in kitty so that we don't need this patch?

@filalex77

This comment has been minimized.

Copy link
Contributor

filalex77 commented Dec 3, 2019

Testing build on darwin
@GrahamcOfBorg build kitty

Copy link
Contributor

filalex77 left a comment

Works on NixOS unstable (tested with nix-review).
printf \\a in kitty makes a sound, no warning about libcanberra.

Still, Darwin build should be checked too.

@Luflosi

This comment has been minimized.

Copy link
Contributor Author

Luflosi commented Dec 3, 2019

Could libEGL.so.1 also be loaded with LD_LIBRARY_PATH?

Copy link
Contributor

jtojnar left a comment

One of the reasons we prefer patches to environment variables is that env vars get inherited by child programs. This is especially bad with terminal emulator, which will be running other programs. LD_LIBRARY_PATH will take precedence over their DT_RUNPATH entries, which Nix heavily relies on.

Copy link
Contributor

jtojnar left a comment

One of the reasons we prefer patches to environment variables is that env vars get inherited by child programs. This is especially bad with terminal emulator, which will be running other programs. LD_LIBRARY_PATH will take precedence over their DT_RUNPATH entries, which Nix heavily relies on.

Copy link
Contributor

jtojnar left a comment

One of the reasons we prefer patches to environment variables is that env vars get inherited by child programs. This is especially bad with terminal emulator, which will be running other programs. LD_LIBRARY_PATH will take precedence over their DT_RUNPATH entries, which Nix heavily relies on.

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Dec 3, 2019

++ to @jtojnar requested changes here. I actually ran into issues because my preferred terminal program had a wrapper with LD_LIBRARY_PATH, it was pretty confusing until I figured this out.

Though this isn't meant to trivialize @Ma27 generosity for helping here ❤️

Also about #74874 (comment), you can split the patch into a separate file and apply it conditionally.

@Ma27

This comment has been minimized.

Copy link
Member

Ma27 commented Dec 3, 2019

One of the reasons we prefer patches to environment variables is that env vars get inherited by child programs. This is especially bad with terminal emulator, which will be running other programs. LD_LIBRARY_PATH will take precedence over their DT_RUNPATH entries, which Nix heavily relies on.

I see, thanks for elaborating! I can confirm that the author's change fixes the issue, so I'd suggest I alter the patch to apply the librarie's changes on Linux only, then this should be good to go.

As the issue exists on release-19.09 as well (the libcanberra dependency was introduced somewhere in 0.14), I'd also do the backport :)

@Ma27 Ma27 force-pushed the Luflosi:fix-kitty-lib branch from ad5b473 to 78f7187 Dec 3, 2019
Copy link
Member

Ma27 left a comment

The original change fixes the issue, I pushed a fix to ensure that stuff like libcanberra is only patched on linux. @jtojnar anything to add? :)

@ofborg ofborg bot requested a review from Ma27 Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.