python311Packages.cython: restrict patch to python versions where the API exists

[voidus]

This doesn't actually cause python311Packages.Cython to fail it's build but python311Packages.skia-pathops

Description of changes

Cython used internal python APIs for a while, but that was removed at some point. We added a patch to add that back in in e69d0a0 but since the API was removed in python3.11 we cannot use the patch anymore.

I can't confidently say I understand why the patch was added so I don't want to speak on whether it is still needed, but since it doesn't work on python3.11 we should probably merge this?

Pinging

• @collares Since you added the patch
• @FRidh as Cython maintainer
• @barinov274 as skia-pathops maintainer

Things done

Only use the patch on python version <=3.11

• Built on platform(s)
  • x86_64-linux (Cython and skia-pathops)
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.11 Release Notes (or backporting 22.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[collares]

Can you apply cython/cython@e337825 atop the currently-applied patch instead of removing it? It effectively reverts the changes in Cython/Utility/ExtensionTypes.c (because they were upstreamed to Python 3.8+) but does not revert the Cython/Compiler/* changes, meaning that Cython will still use trashcan support (the upstream version now). Thanks!

The observable effect of removing the patch would be stack overflows when running Sage tests, with backtraces similar to the one in https://trac.sagemath.org/ticket/27267.

[collares]

For reference, the original patch (the one that would be removed if this PR were merged right now) was committed as cython/cython@f781880. I'm pointing this out because the patch is currently fetched from Sage's repository, which might make code archaeology unnecessarily hard.

Since the 2019 date on the above commit may be surprising, note that both patches were committed to Cython master, which eventually will become Cython 3.0. Since November 2018, Cython releases come from the 0.29.x branch.

[voidus]

I thought it was uppercase Cython for some reason, maybe we need to trigger a rebuild? I'm not sure how ofborg works.

[collares]

You'll probably want to target staging (or python-updates, if there's an open Python Updates PR, but I couldn't find one at a quick glance), because Cython is a transitive dependency of many many packages.

@ofborg build python311Packages.cypari2

[collares]

I thought it was uppercase Cython for some reason, maybe we need to trigger a rebuild? I'm not sure how ofborg works.

ofBorg looks at the commit title, not at the PR title. It will automatically trigger a new build on push (even force pushes).

[voidus]

Added the second patch as you suggested. I'm also pulling in both of them from the official repo, let me know/revert the lines if you'd prefer to leave the old one as it is.

@ofborg build python311Packages.skia-pathops

[collares]

I've extended the comment a bit, but I am happy with this change if it fixes the skia-pathops problem. Please switch the target branch to staging, though.

[voidus]

Done and done :)

[collares]

Great, many thanks for updating the patches! And thanks for pinging me in the first place, I probably would have missed it. Let's wait for the Cython maintainer's approval, but please ping me if that hasn't happened in a reasonable timeframe (a week, say).

[SuperSandro2000]

fails to build:

gcc -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -I/nix/store/jwg037i9jji90ap1112wd5hb5f4lg9iy-libxcrypt-4.4.28/include -fPIC -I./cypari2 -I/nix/store/rhqyzfxyk43gmr4yz1w9b7b8h2mz4r4m-python3.11-cysignals-1.11.2/lib/python3.11/site-packages/cysignals -I/nix/store/zy158n41isy1l8asc4q6kgz4bj255py8-pari-2.13.4/include -I/nix/store/s2q0x350276vgsqls8ws317kcrdii2xk-python3-3.11.0/include/python3.11 -c cypari2/convert.c -o build/temp.linux-x86_64-cpython-311/cypari2/convert.o
cypari2/convert.c: In function ‘__pyx_f_7cypari2_7convert_PyLong_FromINT’:
cypari2/convert.c:3395:21: error: lvalue required as unary ‘&’ operand
 3395 |   __pyx_v_sizeptr = &Py_SIZE(((PyObject *)__pyx_v_x));
      |                     ^
error: command '/nix/store/mjfccdxzpb87wa4pz56fkv6l63pw9in4-gcc-wrapper-11.3.0/bin/gcc' failed with exit code 1
error: builder for '/nix/store/g5ibxzw5d297q3n36qrvgv2n5zw1lsff-python3.11-cypari2-2.1.2.drv' failed with exit code 1;

[collares]

@SuperSandro2000 Thanks for the heads up! I tested cypari2 out of an abundance of caution, but it turns out to be incompatible with Python 3.11 for reasons unrelated to this PR (sagemath/cypari2#114). It was fixed upstream (sagemath/cypari2#120), but I think we can afford to wait for a new cypari2 release since Python 3.11 is opt-in in Nixpkgs for now. Either way, the compilation problem is caused solely by Python 3.11 and not by this PR.

[SuperSandro2000]

@ofborg build python311Packages.cython