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

itstool: 2.0.2 -> 2.0.6 #72342

Merged
merged 2 commits into from Nov 1, 2019

Conversation

@chkno
Copy link
Contributor

chkno commented Oct 31, 2019

Motivation for this change

To get python3 support. #63174 flipped itstool to python3, but itstool doesn't support python3 until 2.0.3 (and perhaps does not support it well until 2.0.5).

Pressing forward instead of rolling back at worldofpeace's suggestion in #63174 and #72335, where it is mentioned that other distros seem to be able to ship recent versions of itstool.

Tensions in this space seem two-fold. One set centers around libxml2 being a low-level C library with sharp edges, manual memory management, and performance concerns; the python libxml2 wrapper being quite thin (the most dubious character in this drama); and python's sentiment that it ought to be quite hard to crash the interpreter casually. This comes to a head in https://gitlab.gnome.org/GNOME/libxml2/issues/12 , where a use-after-free problem in idiomatic-looking python code is declared working-as-designed.

The other set is around python3 being more UTF-8-aware than libxml2's python wrapper, such as https://bugzilla.gnome.org/show_bug.cgi?id=789714 and https://src.fedoraproject.org/rpms/libxml2/blob/master/f/libxml2-2.9.8-python3-unicode-errors.patch

itstool is caught in this crossfire merely for being a widely-used python program that uses XML.

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" (This is 4,000 packages, if I'm counting correctly. It would take a long time for me to run this to completion on my personal hardware)
  • 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 @jtojnar @worldofpeace @FRidh

To get python3 support.  #63174 flipped itstool to python3, but itstool
doesn't support python3 until 2.0.3 (and perhaps does not support it
well until 2.0.5).

Pressing forward instead of rolling back at worldofpeace's suggestion,
who mentions that other distros seem to be able to ship recent versions
of itstool.

Tensions in this space seem two-fold.  One set centers around libxml2
being a low-level C library with sharp edges, manual memory management,
and performance concerns; the python libxml2 wrapper being quite thin
(the most dubious character in this drama); and python's sentiment that
it ought to be quite hard to crash the interpreter casually.  This comes
to a head in https://gitlab.gnome.org/GNOME/libxml2/issues/12 , where a
use-after-free problem in idiomatic-looking python code is declared
working-as-designed.

The other set is around python3 being more UTF-8-aware than libxml2's
python wrapper, such as https://bugzilla.gnome.org/show_bug.cgi?id=789714
and https://src.fedoraproject.org/rpms/libxml2/blob/master/f/libxml2-2.9.8-python3-unicode-errors.patch

itstool is caught in this crossfire merely for being a widely-used
python program that uses XML.
@chkno

This comment has been minimized.

Copy link
Contributor Author

chkno commented Oct 31, 2019

If this goes forward, #72335 (revert to python2) should not. I don't know which is all-around best. My interest here is just to be able to run NixOS tests on staging.

@chkno chkno mentioned this pull request Oct 31, 2019
4 of 10 tasks complete
@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Oct 31, 2019

We will need to apply the opensuse patch/hack for this to work.

@chkno

This comment has been minimized.

Copy link
Contributor Author

chkno commented Oct 31, 2019

Maybe. I was able to reproduce the problem building gnome3.gnome_desktop fd9cfc4 describes with itstool 2.0.4, but I was not able to reproduce it with itstool 2.0.6. I.e., if 2.0.6 had been available when fd9cfc4 was committed, bumping to 2.0.6 (and python3) instead of reverting to 2.0.2 would have fixed the gnome3.gnome_desktop build problem just as well.

@chkno

This comment has been minimized.

Copy link
Contributor Author

chkno commented Oct 31, 2019

fd9cfc4 mentions that other packages were broken as well, but doesn't say which ones. #41616 , which that fix is attached to, is about the nixos.tests.gnome3 test.

I cannot build the gnome3 NixOS test with this PR. I get this error while trying to build gnome3.zenity, one of its dependencies:

Traceback (most recent call last):
  File "/nix/store/0ri31bhv3n57pxqaky3s8sqz72yqay7s-itstool-2.0.6/bin/itstool", line 27, in <module>
    import libxml2
  File "/nix/store/gf8s6kl3v1fgqaz52qd2ffnvja9s8rjv-libxml2+py-2.9.9/lib/python2.7/site-packages/libxml2.py", line 1, in <module>
    import libxml2mod
ImportError: dynamic module does not define module export function (PyInit_libxml2mod)
make[2]: *** [Makefile:554: bg/bg.stamp] Error 1
make[2]: Leaving directory '/build/zenity-3.32.0/help'
make[1]: *** [Makefile:424: all-recursive] Error 1
make[1]: Leaving directory '/build/zenity-3.32.0'
make: *** [Makefile:365: all] Error 2
builder for '/nix/store/nixp439kx1acw8s0nxbbssky118fhc8b-zenity-3.32.0.drv' failed with exit code 2
error: build of '/nix/store/nixp439kx1acw8s0nxbbssky118fhc8b-zenity-3.32.0.drv' failed

I can build zenity with the itstool-back-to-python2 PR #72335

@FRidh

This comment has been minimized.

Copy link
Member

FRidh commented Oct 31, 2019

i have the impression Python 2 and 3 get mixed up in that trace. It's using libxml2Python which is Python2.

@chkno Could you push a commit with the patch here as well?

@chkno

This comment has been minimized.

Copy link
Contributor Author

chkno commented Oct 31, 2019

No.

  • I haven't seen any harm come of the problem described in https://bugzilla.opensuse.org/show_bug.cgi?id=1065270 other than the test case posted to that bug. The issue that caused nixpkgs to revert itstool from 2.0.4 to 2.0.2 is resolved just as well by bumping to 2.0.6, which is a much cleaner fix than patching a different, much more widely used library to resolve a problem in itstool. (edit: gnumeric is broken too)
  • No attempt by any party has been made to upstream that patch. That openSUSE bug was incorrectly marked a duplicate of another, only slightly related bug, which probably forestalled any attempt to push it upstream from there.
  • https://bugzilla.gnome.org/show_bug.cgi?id=789714 notes that there are several other UTF-8 problems not addressed in that patch.
  • Most of all: That patch calls PyErr_Clear() in an error-handling channel. That seems potentially problematic to me. I don't know this code well enough to evaluate the comment claiming that this is a benign thing to do, and digging in to find out would overflow my yak-shaving stack.

This change in pkgs/development/libraries/libxml2/default.nix applies that patch, but I don't feel comfortable authoring and proposing a git commit that does this:

+  patches = [ (fetchurl {
+    url = "https://bugzilla.opensuse.org/attachment.cgi?id=746044";
+    sha256 = "37eb81a8ec6929eed1514e891bff2dd05b450bcf0c712153880c485b7366c17c";
+  }) ];

(edit: c0cecd0 below proposes this patch without the PyErr_Clear() and with an explanation of why a band-aid is appropriate rather than trying for a proper fix.)

@chkno

This comment has been minimized.

Copy link
Contributor Author

chkno commented Oct 31, 2019

The zenity build failure is not related to itstool. Zenity builds just fine when the itstool 2.0.6 bump is applied immediately after the 2.0.4 -> 2.0.2 revert. I think this command will find the commit that introduced the zenity build failure:

git bisect start 6b7f343121b3d77a1c17532c900f8aa40ad8c5e2 4b649a99d8461c980e7028a693387dc48033c1f7
git bisect run bash -c 'git log -n1 -p 0c999c7521ee52dd00566643f3600627783d2550 | patch -p1 && nix-build -A gnome3.zenity -I nixpkgs=.; status=$?; git checkout pkgs/development/tools/misc/itstool/default.nix; exit $status'
@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Oct 31, 2019

@GrahamcOfBorg build gnome3.gnome-devel-docs gnumeric

@FRidh

This comment has been minimized.

Copy link
Member

FRidh commented Oct 31, 2019

ok, the zenity build failure is because it uses gnome-doc-utils which is a Python 2 application, which depends on libxml2Python and propagates it. This ends up in zenity as a build time dependency, causing a mixture of Python 2 and 3 libraries.

Python libraries and especially applications ideally should not propagate their dependencies, but we're not there yet. We should be able to work around this by adding libxml2Python to pythonPath instead of propagatedBuildInputs.

@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Oct 31, 2019

Could we do the same thing meson does for now?

rm $out/nix-support/propagated-build-inputs

@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Oct 31, 2019

With gnumeric, I am still getting the segfault on your branch.

… crash

1. Gnumeric has unbalanced XML tags in its doc translations.
2. itstool's XML error handler tries to print this error with context.
3. libxml2's context snipper treats the data as bytes, not UTF-8.
4. python3Packages.libxml2 casts the context to a UTF-8 Python string.
5. itstool dereferences a null pointer.

This patch intervenes at #4.

In https://bugzilla.gnome.org/show_bug.cgi?id=789714#c4 , upstream
suggests that intervening at #3 would be better -- that each of the four
copies of xmlParserPrintFileContextInternal() have four additional UTF-8
problems, one of which is that the caret indicator ought to count
"unicode characters" not bytes.  But to position a caret correctly, a
character count is not sufficient -- this would need to use icu's BiDi
logic (with fallback to doing something wrong when libxml2 is configured
not to use icu) -- which makes a 'correct' fix a much larger project
than this simple band-aid.
@chkno

This comment has been minimized.

Copy link
Contributor Author

chkno commented Nov 1, 2019

(The bisect finished: a2c62e2 introduced the zenity build break.)

gnumeric -- thanks! Yes, that one is broken. And that helped me find more upstream issues tracking this: itstool/itstool#22 and https://gitlab.gnome.org/GNOME/libxml2/issues/64

I captured a valgrind --tool=memcheck trace of this problem:

Process terminating with default action of signal 11 (SIGSEGV)
 Access not within mapped region at address 0x0
   at 0x48B00F4: function_code_fastcall (in /nix/store/w0rck0g2i12c7zmmcmngfpnszc5238ry-python3-3.7.4/lib/libpython3.7m.so.1.0)
   by 0x48E2116: _PyFunction_FastCallDict (in /nix/store/w0rck0g2i12c7zmmcmngfpnszc5238ry-python3-3.7.4/lib/libpython3.7m.so.1.0)
   by 0xD8D438D: libxml_xmlErrorFuncHandler (in /nix/store/p8i1sm5x9bx6967f59fjb63rlm28iajh-libxml2-2.9.9-py/lib/python3.7/site-packages/libxml2mod.so)
   by 0xD948214: xmlParserPrintFileContextInternal (in /nix/store/kyi0j76yy6pnpz55cysx95a1lffp1wlz-libxml2-2.9.9/lib/libxml2.so.2.9.9)
   by 0xD94857B: xmlReportError (in /nix/store/kyi0j76yy6pnpz55cysx95a1lffp1wlz-libxml2-2.9.9/lib/libxml2.so.2.9.9)
   by 0xD94A06C: __xmlRaiseError (in /nix/store/kyi0j76yy6pnpz55cysx95a1lffp1wlz-libxml2-2.9.9/lib/libxml2.so.2.9.9)
   by 0xD94E6C4: xmlFatalErrMsgStrIntStr (in /nix/store/kyi0j76yy6pnpz55cysx95a1lffp1wlz-libxml2-2.9.9/lib/libxml2.so.2.9.9)
   by 0xD95C2AB: xmlParseEndTag2 (in /nix/store/kyi0j76yy6pnpz55cysx95a1lffp1wlz-libxml2-2.9.9/lib/libxml2.so.2.9.9)
   by 0xD960DA5: xmlParseElement (in /nix/store/kyi0j76yy6pnpz55cysx95a1lffp1wlz-libxml2-2.9.9/lib/libxml2.so.2.9.9)
   by 0xD960445: xmlParseContent (in /nix/store/kyi0j76yy6pnpz55cysx95a1lffp1wlz-libxml2-2.9.9/lib/libxml2.so.2.9.9)
   by 0xD960D42: xmlParseElement (in /nix/store/kyi0j76yy6pnpz55cysx95a1lffp1wlz-libxml2-2.9.9/lib/libxml2.so.2.9.9)
   by 0xD960445: xmlParseContent (in /nix/store/kyi0j76yy6pnpz55cysx95a1lffp1wlz-libxml2-2.9.9/lib/libxml2.so.2.9.9)
...

The PyErr_Clear() is not needed to get Gnumeric to build, so I left it out.

I also created a gnumeric PR to fix the malformed XML that's causing these errors that itstool / python3Packages.libxml2 fails to handle gracefully.

jtojnar added a commit to jtojnar/nixpkgs that referenced this pull request Nov 1, 2019
That breaks packages that rely on Python like itstool does.

NixOS#72342 (comment)
@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Nov 1, 2019

a2c62e2 looks unlikely to cause that. I think it is caused by #63147 as well.

Anyway, I confirmed that https://github.com/jtojnar/nixpkgs/tree/zenity-fix fixes that instance.

I would say that if you cherry-pick that and add the GNumeric patch we can merge this. We can always deal with the remaining packages later.

@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Nov 1, 2019

Or since you added the libxml hack, the GNumeric one is not necessary.

@jtojnar
jtojnar approved these changes Nov 1, 2019
Copy link
Contributor

jtojnar left a comment

Or let's just merge this and I will handle the zenity fix separately.

@jtojnar jtojnar merged commit ad1f06f into NixOS:staging Nov 1, 2019
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
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
@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Nov 1, 2019

Thanks.

jtojnar added a commit that referenced this pull request Nov 1, 2019
That breaks packages that rely on Python like itstool does.

#72342 (comment)
@chkno chkno deleted the chkno:itstool-2.0.6 branch Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.