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.

Sign up for GitHub

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

Jump to bottom

signal-desktop: 1.29.6 -> 1.30.0 #78413

Merged
merged 1 commit into from Jan 24, 2020
Merged

signal-desktop: 1.29.6 -> 1.30.0 #78413

merged 1 commit into from Jan 24, 2020

Conversation

primeos
Copy link
Member

@primeos primeos commented Jan 24, 2020 •
edited

Changelog: https://github.com/signalapp/Signal-Desktop/releases/tag/v1.30.0

Current problem

This currently fails with the following error message:

/run/current-system/sw/bin/signal-desktop: symbol lookup error: /nix/store/8n103q18ip21zjl0xh36618291h6bkag-pango-1.44.7/lib/libpangoft2-1.0.so.0: undefined symbol: pango_font_get_hb_font

The problem seems to be that signal-desktop is looking in the wrong place as this symbol is defined in libpango-1.0.so:

$ nm -D /nix/store/8n103q18ip21zjl0xh36618291h6bkag-pango-1.44.7/lib/libpangoft2-1.0.so.0 | grep pango_font_get_hb_font
                 U pango_font_get_hb_font
$ nm -D /nix/store/8n103q18ip21zjl0xh36618291h6bkag-pango-1.44.7/lib/libpango-1.0.so | grep pango_font_get_hb_font
0000000000018220 T pango_font_get_hb_font

But the new autoPatchelfHook abstraction (#77850) seems to select the wrong shared library (a bundled one instead of the system library):

$ ldd /nix/store/jjlnp1yid3bcj8yyj0jlr2ykpy30ivnq-signal-desktop-1.30.0/bin/.signal-desktop-wrapped | grep pango
	libpangocairo-1.0.so.0 => /nix/store/jjlnp1yid3bcj8yyj0jlr2ykpy30ivnq-signal-desktop-1.30.0/lib/Signal/resources/app.asar.unpacked/node_modules/sharp/vendor/lib/libpangocairo-1.0.so.0 (0x00007f8e88264000)
	libpango-1.0.so.0 => /nix/store/jjlnp1yid3bcj8yyj0jlr2ykpy30ivnq-signal-desktop-1.30.0/lib/Signal/resources/app.asar.unpacked/node_modules/sharp/vendor/lib/libpango-1.0.so.0 (0x00007f8e88014000)
	libpangoft2-1.0.so.0 => /nix/store/8n103q18ip21zjl0xh36618291h6bkag-pango-1.44.7/lib/libpangoft2-1.0.so.0 (0x00007f8e864bc000)

Which doesn't contain the required symbol (added in version 1.44):

$ nm -D /nix/store/jjlnp1yid3bcj8yyj0jlr2ykpy30ivnq-signal-desktop-1.30.0/lib/Signal/resources/app.asar.unpacked/node_modules/sharp/vendor/lib/libpango-1.0.so.0 | grep pango_font_get_hb_font || echo "No match"
No match

@worldofpeace: Any ideas what to do here? I guess we could e.g. manually change the search path, but maybe there's a better solution for such cases.

Update (also relevant): Signal/resources/app.asar.unpacked/node_modules/sharp is new in version 1.30.0: signalapp/Signal-Desktop#3919

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 nixpkgs-review --run "nixpkgs-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.

@ofborg ofborg bot requested review from ixmatus and equirosa January 24, 2020 14:07
@worldofpeace
Copy link
Contributor

But the new autoPatchelfHook abstraction (#77850) seems to select the wrong shared library (a bundled one instead of the system library):

I believe you can remove the bundled library before the patching hook runs.
And it would use the system library.

@primeos primeos marked this pull request as ready for review January 24, 2020 16:51
@primeos
Copy link
Member Author

primeos commented Jan 24, 2020

I believe you can remove the bundled library before the patching hook runs.
And it would use the system library.

Considered doing this but it seems a bit dangerous to me. There could be problems due to incompatible versions (should normally not be a problem, but who knows...), we would need to add a lot (didn't count them, maybe it wouldn't be that bad) of libraries, and most importantly resources/app.asar (which we cannot simply patch) contains references like this one:

"libpangocairo-1.0.so":{"link":"node_modules/sharp/vendor/lib/libpangocairo-1.0.so.0.4200.4"}

which makes me assume that we cannot simply delete these files (but it might fallback to the system-libraries).

Anyway, I had a look at pkgs/build-support/setup-hooks/auto-patchelf.sh and decided to use the no-recourse option for now to avoid adding and patching those libraries.

That solution seems to work well so far as we mainly need to patch signal-desktop. But I'm open to PRs if someone finds a better solution.

@GrahamcOfBorg test signal-desktop

@primeos
Copy link
Member Author

primeos commented Jan 24, 2020

Should be good to go. The tests are still fine and I've also tested the super important (/s) new Create/Upload Sticker Pack feature (used a Google search for tux filetype:png imagesize:512x512 to obtain some test images).

@primeos primeos merged commit 3791770 into NixOS:master Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ixmatus ixmatus Awaiting requested review from ixmatus

@equirosa equirosa Awaiting requested review from equirosa

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@primeos @worldofpeace