Skip to content

pango, pygtk: fixing up newer pango and pygtk - #74282

Merged
jtojnar merged 4 commits into
NixOS:stagingfrom
d-goldin:pango_update_split
Dec 3, 2019
Merged

jtojnar merged 4 commits into
NixOS:stagingfrom
d-goldin:pango_update_split

Conversation

@d-goldin

@d-goldin d-goldin commented Nov 26, 2019 •

Copy link
Copy Markdown
Contributor
Motivation for this change

We are currently on a rather old version of pango and are being kept back by pygtk, which in some
sort of abandoned-but-beloved limbo. This proposes a split into two pango versions to be able to move
the rest of the ecosystem along and just pin pygtk until we know what to do about it.

Edit: After the below discussion, we decided to not split and instead patch up pygtk.

This is related to some discussion in #71571 from a while ago.

Unfortunately, due to the size of the rebuild, I'm unable to rebuild this on my machine, so I'd be thankful if somebody could do a nix-review run.

Right now this requires a patch to cmake to correctly locate harfbuzz: https://gitlab.kitware.com/cmake/cmake/issues/19531 until we get 3.16 in (it was just released today, but has quite a lot of changes, so I'm leaving the patch in for now instead of bumping the version).

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 @7c6f434c as maintainer and @jtojnar because of context

@d-goldin d-goldin changed the title Pango update split pango: split into new and old version Nov 26, 2019
@ofborg ofborg Bot added 6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 8.has: package (new) This PR adds a new package labels Nov 26, 2019
@ofborg ofborg Bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Nov 26, 2019
Comment thread pkgs/development/libraries/pango/1.43.nix Outdated
@worldofpeace

worldofpeace commented Nov 26, 2019 •

Copy link
Copy Markdown
Contributor

I don't think this will work because I tried it. You'd have to give pygtk it's own version of gtk overridden to use an older version of pango. And that's still a pretty poor solution, I didn't PR it.

Edit: though I'm not opossed to this anymore. I'd consider those applications that still use this abandoned library unmaintained. So tier of quality... it won't matter to much there's another version of gtk2 for this.
(though there's gimp...)

@worldofpeace

worldofpeace commented Nov 26, 2019 •

Copy link
Copy Markdown
Contributor

I also think there's probably a lot of packages we could either remove/update to not use pygtk.
I believe bleachbit doesn't even need it anymore, so that's one popular application.

Edit: have wip work for this.

@d-goldin

Copy link
Copy Markdown
Contributor Author

@worldofpeace: Thanks for the pointers, I'll look into it.

@worldofpeace

Copy link
Copy Markdown
Contributor

Opened #74295 to remove some pygtk users that are also probably abandoned.

@jtojnar

jtojnar commented Nov 26, 2019

Copy link
Copy Markdown
Member

pygtk should build now, according to #65670 (comment)

@worldofpeace

Copy link
Copy Markdown
Contributor

pygtk should build now, according to #65670 (comment)

Hmm forgot about. Looking through my branches when I did my testing, I didn't look like I succeeded in getting it to build even with that.

@d-goldin

d-goldin commented Nov 27, 2019 •

Copy link
Copy Markdown
Contributor Author

pygtk should build now, according to #65670 (comment)

I must admit, this issue has been kinda sitting in my stash for a while, so I might need to refresh my memory a bit. I think I saw this commit "back then" too. I tried this now again, and think to remember that they for some reason did not bring back a part of the API. Building pygtk with the most recent pango 1.44.7 release I'm getting the following (without any adjustments to pygtk), even though there was a remark about a fix;

[...]
pango.c: In function ‘_wrap_PangoFont__do_find_shaper’:
pango.c:3822:32: error: ‘PangoFontClass’ {aka ‘struct _PangoFontClass’} has no member named ‘find_shaper’
     if (PANGO_FONT_CLASS(klass)->find_shaper)
                                ^~
pango.c:3823:38: error: ‘PangoFontClass’ {aka ‘struct _PangoFontClass’} has no member named ‘find_shaper’
         ret = PANGO_FONT_CLASS(klass)->find_shaper(PANGO_FONT(self->obj), lang, ch);
[...]

Maybe I'm missing something.

@matthiasclasen: If possible, could you please advise? Were you able to compile pygtk 2.24.0 with 1.44.7?

@d-goldin

Copy link
Copy Markdown
Contributor Author

Opened #74295 to remove some pygtk users that are also probably abandoned.

As far as we can, for sure! I was viewing this more as a defensive stance for the cases where we might not be able to, but maybe it's just pessimism. :)

@jtojnar

jtojnar commented Nov 27, 2019

Copy link
Copy Markdown
Member

Something in this vein might work:

diff --git a/pkgs/development/python-modules/pygtk/default.nix b/pkgs/development/python-modules/pygtk/default.nix
index 09ccb5c3d95..6741ac695b4 100644
--- a/pkgs/development/python-modules/pygtk/default.nix
+++ b/pkgs/development/python-modules/pygtk/default.nix
@@ -1,4 +1,4 @@
-{ stdenv, fetchurl, python, pkgconfig, gtk2, pygobject2, pycairo
+{ stdenv, fetchurl, fetchpatch, python, pkgconfig, gtk2, pygobject2, pycairo
 , buildPythonPackage, libglade ? null, isPy3k }:
 
 buildPythonPackage rec {
@@ -12,6 +12,19 @@ buildPythonPackage rec {
     sha256 = "04k942gn8vl95kwf0qskkv6npclfm31d78ljkrkgyqxxcni1w76d";
   };
 
+  patches = [
+    # not sure what this does but Fedora and Arch have it
+    (fetchpatch {
+      url = "https://src.fedoraproject.org/rpms/pygtk2/raw/93b36f49fdf19b3673c04f2f85310e88b1f0fcc6/f/0001-Fix-leaks-of-Pango-objects.patch";
+      sha256 = "031px4w5cshcx1sns430sdbr2i007b9zyb2carb3z65nzr77dpdd";
+    })
+  ];
+
+  postPatch = ''
+    # regenerate pango defs so it builds
+    python2 $(pkg-config pygobject-2.0 --variable codegendir)/h2def.py $(pkg-config pango --variable includedir)/pango-1.0/pango/*.h > pango.defs
+  '';
+
   nativeBuildInputs = [ pkgconfig ];
   buildInputs = stdenv.lib.optional (libglade != null) libglade;
 

@d-goldin

d-goldin commented Nov 27, 2019 •

Copy link
Copy Markdown
Contributor Author

I don't think this will work because I tried it. You'd have to give pygtk it's own version of gtk overridden to use an older version of pango. And that's still a pretty poor solution, I didn't PR it.

So, I'm currently trying this, specifically for gimp and the rabbit hole kinda opens a little bit deeper. Adwaita does not seem to build with the newer pango, at least "trivially". Gimp itself at least compiles though, but then lacks themes, gimp-with-plugins which pulls adwaita in, does not.

[...]
for file in `cd ../../Adwaita/scalable; find . -name "*.svg"`; do \
        context="`dirname $file`"; \
        /nix/store/g83k1kbjimimhbrk9nbl66f9nlwxqfyc-coreutils-8.31/bin/mkdir -p /nix/store/j36rdrd095gsg6lqlq05cym86mmlgiyn-adwaita-icon-theme-3.34.3/share/icons/Adwaita/scalable/$context; \
        /nix/store/pn3mrlw8l0pda4w5knfima4pq99glzvw-bash-4.4-p23/bin/bash /build/adwaita-icon-theme-3.34.3/install-sh -c -m 644 ../../Adwaita/scalable/$file /nix/store/j36rdrd095gsg6lqlq05cym86mmlgiyn-adwaita-icon-theme-3.34.3/share/icons/Adwaita/scalable/$file; \
        for size in 16x16 24x24 32x32 48x48 64x64 96x96; do \
                /nix/store/g83k1kbjimimhbrk9nbl66f9nlwxqfyc-coreutils-8.31/bin/mkdir -p /nix/store/j36rdrd095gsg6lqlq05cym86mmlgiyn-adwaita-icon-theme-3.34.3/share/icons/Adwaita/$size/$context; \
                /nix/store/35z14n5z8k31mb0qm2qqzg159a6557m3-gtk+3-3.24.12-dev/bin/gtk-encode-symbolic-svg ../../Adwaita/scalable/$file $size -o /nix/store/j36rdrd095gsg6lqlq05cym86mmlgiyn-adwaita-icon-theme-3.34.3/share/icons/Adwaita/$size/$context; \
        done \
done
Can't load file: Unrecognized image file format
Can't load file: Unrecognized image file format
Can't load file: Unrecognized image file format
Can't load file: Unrecognized image file format
Can't load file: Unrecognized image file format
Can't load file: Unrecognized image file format
[... ultimately fails due to that ...]

Also, maybe interesting to keep an eye on, even though there was no apparent activity in 10 months: https://gitlab.gnome.org/GNOME/gimp/issues/339

@jtojnar

jtojnar commented Nov 27, 2019

Copy link
Copy Markdown
Member

I believe https://gitlab.gnome.org/GNOME/gimp/issues/339 is already solved on master (#67576).

@worldofpeace

Copy link
Copy Markdown
Contributor

@jtojnar Ahh right ✨ The postPatch there is probably what I was missing and the issue.

@jtojnar

jtojnar commented Nov 27, 2019

Copy link
Copy Markdown
Member

Does not seem to generate functions for me, though.

@jtojnar

jtojnar commented Nov 27, 2019

Copy link
Copy Markdown
Member

Or just drop the single virtual call https://github.com/flathub/org.glimpse_editor.Glimpse/blob/master/patches/pygtk-Drop-the-PangoFont-find_shaper-virtual-method.patch. Alatiera is planning an upstream MR.

@d-goldin

Copy link
Copy Markdown
Contributor Author

I still need to look into Adwaita and whatever else comes up. If somebody has the resources to run a review build or maybe help test some applications, I'd be thankful.

@jtojnar

jtojnar commented Dec 1, 2019

Copy link
Copy Markdown
Member

adwaita is currently failing on staging, running bisect.

@jtojnar

jtojnar commented Dec 1, 2019

Copy link
Copy Markdown
Member

That is just weird:

Bisecting: 0 revisions left to test after this (roughly 0 steps)
[0a443f4c72595d0fc1b678ad4f511429c13c8c65] kodi: drop jasper dependency
running ./bisect.sh
/nix/store/5sjl4azzscizxvhv0m78v0ivw26q16y5-adwaita-icon-theme-3.34.0
9aa62321ea9c2f7a355d4c549cf1f803c0283ca8 is the first bad commit
commit 9aa62321ea9c2f7a355d4c549cf1f803c0283ca8
Author: c0bw3b <c0bw3b@users.noreply.github.com>
Date:   Sun Nov 17 19:44:10 2019 +0100

    gdk-pixbuf: disable JPEG2000 support
    
    jasper has unfixed CVE
    Upstream has no plan to switch to openjpeg AFAICT

 pkgs/development/libraries/gdk-pixbuf/default.nix | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
bisect run success

#73586

That is just weird as jasper did not propagate anything other than libjpeg, which is already a dependency of gdk-pixbuf.

That should have no bearing on MIME sniffing.

@jtojnar jtojnar closed this Dec 1, 2019
@jtojnar

jtojnar commented Dec 1, 2019

Copy link
Copy Markdown
Member

Continuing the discussion in #73586

@jtojnar jtojnar reopened this Dec 1, 2019
@d-goldin

d-goldin commented Dec 1, 2019

Copy link
Copy Markdown
Contributor Author

Note for later: We probably want a release-notes entry and possibly doc adjustments to that one, regarding deprecated bitmap fonts.

@jtojnar

jtojnar commented Dec 1, 2019

Copy link
Copy Markdown
Member

Would wording like this work:

Pango was upgraded to 1.44, which no longer uses freetype for font loading. This means that type1 and bitmap fonts are no longer supported in applications relying on Pango for font rendering (notably, GTK application).
See <link xlink:href="https://gitlab.gnome.org/GNOME/pango/issues/386">upstream issue</link> for more information.

@d-goldin

d-goldin commented Dec 1, 2019

Copy link
Copy Markdown
Contributor Author

Would wording like this work:

Pango was upgraded to 1.44, which no longer uses freetype for font loading. This means that type1 and bitmap fonts are no longer supported in applications relying on Pango for font rendering (notably, GTK application).
See <link xlink:href="https://gitlab.gnome.org/GNOME/pango/issues/386">upstream issue</link> for more information.

I think the wording is good, but maybe we can find some other resource to link. I remember that I once had some release note PR and included the link to another PR in the description and the reviewers did not think linking to a nixpkgs PR was the best option. What do you think of maybe using https://blogs.gnome.org/mclasen/2019/05/25/pango-future-directions/ ?

@jtojnar

jtojnar commented Dec 2, 2019

Copy link
Copy Markdown
Member

IMHO the issue is better resource as it provides more details (including discussion about converting bitmap fonts). Only a small part of the blog post is dedicated to this and the link is in the issue OP for those interested.

@d-goldin

d-goldin commented Dec 2, 2019

Copy link
Copy Markdown
Contributor Author

Alright, personally I'm fine with the issue link, so I'll just add it and then we'll see.

* Removes an unused binding that prevents compilation with newer pango
* Adds a patch to fix a memory leak
a patch to cmake to correctly locate harfbuzz:
https://gitlab.kitware.com/cmake/cmake/issues/19531,
needed for more recent pango.
@ofborg ofborg Bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation labels Dec 2, 2019
@jtojnar

jtojnar commented Dec 3, 2019 •

Copy link
Copy Markdown
Member

Can we wrap this up? It is blocking Nautilus update.

Edit: did not see you already pushed the release notes.

@jtojnar
jtojnar merged commit daa676f into NixOS:staging Dec 3, 2019
@d-goldin

d-goldin commented Dec 3, 2019

Copy link
Copy Markdown
Contributor Author

@jtojnar: From my side, yes, we can give it a go and deal with any unexpected fallout then. I was just waiting on reviewers approval/further change requests.

@d-goldin
d-goldin deleted the pango_update_split branch December 3, 2019 14:31
@d-goldin

d-goldin commented Dec 3, 2019

Copy link
Copy Markdown
Contributor Author

Thanks for the support and help. I will try to watch for upcoming breakages, but otherwise, feel free to ping me.

@worldofpeace

Copy link
Copy Markdown
Contributor

@d-goldin I believe I suggested not linking to our internal tracker or PRs, and to provide a better resource in the release notes. In this case it's an external issue so it's fine to link.

@tobim tobim mentioned this pull request Dec 4, 2019
10 tasks
@jtojnar

jtojnar commented Dec 15, 2019

Copy link
Copy Markdown
Member

Another broken abandoned package is pangox-compat: https://hydra.nixos.org/build/108047288/nixlog/1

Not sure it is worth it trying to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python Python is a high-level, general-purpose programming language. 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants