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

python312Packages.sphinx: re-enable cython test #365705

Open
wants to merge 1 commit into
base: staging
Choose a base branch
from

Conversation

implr
Copy link
Contributor

@implr implr commented Dec 16, 2024

6cad926 updated cython, which fixes the test failure here.

Extracted from #341644.
Didn't test dependent packages, as the set is huge.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@lukegb
Copy link
Contributor

lukegb commented Jan 3, 2025

OOI does this have to be cython_0? The 7.4.7 pyproject.toml claims cython>=3.0, which would be normal cython, I think?

@lukegb
Copy link
Contributor

lukegb commented Jan 3, 2025

OK, this also doesn't work properly under Python 3.12 because cython needs distutils for pyximport (which is used in the test), and this isn't available in stdlib anymore. You can't depend on distutils here because it causes a dependency loop.

My (awful) suggestion is that we instead introduce a dependency on setuptools, which will stub out distutils with its own implementation:

--- a/pkgs/development/python-modules/sphinx/default.nix
+++ b/pkgs/development/python-modules/sphinx/default.nix
@@ -31,7 +31,8 @@
   typing-extensions,
 
   # check phase
-  cython_0,
+  cython,
+  setuptools,
   defusedxml,
   filelock,
   html5lib,
@@ -60,6 +61,12 @@ buildPythonPackage rec {
     hash = "sha256-/5zH9IdLmTGnn5MY4FFSuZOIeF/x1L9Ga/wp57XrAQo=";
   };
 
+  prePatch = ''
+    # Import setuptools before distutils to use setuptools' distutils.
+    # This avoids a dependency loop on Python >=3.12, where distutils was removed from the stdlib.
+    sed -i '/import pyximport/i\    import setuptools' tests/test_extensions/test_ext_autodoc.py
+  '';
+
   build-system = [ flit-core ];
 
   dependencies =
@@ -88,7 +95,8 @@ buildPythonPackage rec {
   __darwinAllowLocalNetworking = true;
 
   nativeCheckInputs = [
-    cython_0
+    cython
+    setuptools
     defusedxml
     filelock
     html5lib

WDYT?

@implr
Copy link
Contributor Author

implr commented Jan 6, 2025

OOI does this have to be cython_0? The 7.4.7 pyproject.toml claims cython>=3.0, which would be normal cython, I think?

Hm, you're right. I was going off the state as it was when it was disabled in nixpkgs, which was a long while ago.

It'd maybe be better if we could break the cycle and depend on distutils somehow, but I don't see an obvious way. The setuptools hack it is then.

6cad926 updated cython, which fixes the test failure here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants