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

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

Merged
merged 4 commits into from Dec 3, 2019

Conversation

d-goldin
Copy link
Contributor

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

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

@worldofpeace
Copy link
Contributor

worldofpeace commented Nov 26, 2019

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
Copy link
Contributor

worldofpeace commented Nov 26, 2019

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
Contributor Author

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

@worldofpeace
Copy link
Contributor

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

@jtojnar
Copy link
Contributor

jtojnar commented Nov 26, 2019

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

@worldofpeace
Copy link
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
Copy link
Contributor Author

d-goldin commented Nov 27, 2019

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
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
Copy link
Contributor

jtojnar commented Nov 27, 2019

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
Copy link
Contributor Author

d-goldin commented Nov 27, 2019

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
Copy link
Contributor

jtojnar commented Nov 27, 2019

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

@worldofpeace
Copy link
Contributor

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

@jtojnar
Copy link
Contributor

jtojnar commented Nov 27, 2019

Does not seem to generate functions for me, though.

@jtojnar
Copy link
Contributor

jtojnar commented Nov 27, 2019

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.

@jtojnar
Copy link
Contributor

jtojnar commented Dec 1, 2019

adwaita is currently failing on staging, running bisect.

@jtojnar
Copy link
Contributor

jtojnar commented Dec 1, 2019

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
Staging automation moved this from Needs review to Done Dec 1, 2019
@jtojnar
Copy link
Contributor

jtojnar commented Dec 1, 2019

Continuing the discussion in #73586

@jtojnar jtojnar reopened this Dec 1, 2019
Staging automation moved this from Done to WIP Dec 1, 2019
@d-goldin
Copy link
Contributor Author

d-goldin commented Dec 1, 2019

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

@jtojnar
Copy link
Contributor

jtojnar commented Dec 1, 2019

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
Copy link
Contributor Author

d-goldin commented Dec 1, 2019

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
Copy link
Contributor

jtojnar commented Dec 2, 2019

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
Copy link
Contributor Author

d-goldin commented Dec 2, 2019

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.
@jtojnar
Copy link
Contributor

jtojnar commented Dec 3, 2019

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
Staging automation moved this from WIP to Done Dec 3, 2019
@d-goldin
Copy link
Contributor Author

d-goldin commented Dec 3, 2019

@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
Copy link
Contributor Author

d-goldin commented Dec 3, 2019

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

@worldofpeace
Copy link
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
Copy link
Contributor

jtojnar commented Dec 15, 2019

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
Projects
Staging
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants