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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

d-goldin commented Nov 26, 2019

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

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Nov 26, 2019

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

@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Nov 26, 2019

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

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Nov 27, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

d-goldin commented Nov 27, 2019

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

worldofpeace commented Nov 27, 2019

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

@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Nov 27, 2019

Does not seem to generate functions for me, though.

@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Nov 27, 2019

@jtojnar

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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.

d-goldin added 4 commits Oct 23, 2019
a patch to cmake to correctly locate harfbuzz:
https://gitlab.kitware.com/cmake/cmake/issues/19531,
needed for more recent pango.
* Removes an unused binding that prevents compilation with newer pango
* Adds a patch to fix a memory leak
@d-goldin d-goldin force-pushed the d-goldin:pango_update_split branch from 8aa178c to c25d4ab Dec 2, 2019
@jtojnar

This comment has been minimized.

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
16 checks passed
16 checks passed
cmake, pango on x86_64-darwin Timed out, unknown build status
Details
cmake, pango on x86_64-linux Timed out, unknown build status
Details
Evaluation Performance Report Evaluator Performance Report
Details
cmake, pango on aarch64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
Staging automation moved this from WIP to Done Dec 3, 2019
@d-goldin

This comment has been minimized.

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 d-goldin:pango_update_split branch Dec 3, 2019
@d-goldin

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

worldofpeace commented Dec 3, 2019

@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
3 of 10 tasks complete
@worldofpeace worldofpeace mentioned this pull request Dec 11, 2019
0 of 24 tasks complete
@jtojnar

This comment has been minimized.

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
You can’t perform that action at this time.