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

python3Packages.setuptools, thrift: Fix setuptools to give thrift python 3.12 support #328415

Closed

Conversation

kfollesdal
Copy link
Contributor

@kfollesdal kfollesdal commented Jul 19, 2024

Description of changes

distutils was removed in python 3.12. setuptools give backwards compatibility for distutils, by a trick/hack that make distutils available/importable when setuptools are installed. But this do not work in nix build phase.

This break some python 3.12 packages builds in nixpkgs. For example the python package thrift is broken for python 3.12 but also other examples #328379.

This PR make the setuptools support for distutils behave as expected, by symlink the distutils package in setuptools to distutils in site-packages.

One concern, this touches setuptools implies big rebuild. But will make setuptools behave as expected and avoid confusions. There are as suggestion in #328379 to add distutils as it own package, but think it is better to fix setuptools, then introducing a packages that is gone.

My earlier proposal:
#328182

Alternativ only fix thrift:
#328246

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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

@kfollesdal kfollesdal changed the title Kfollesdal/setuptools distutils python3Packages.setuptools, thrift: Fix setuptools to give thrift python 3.12 support Jul 19, 2024
@dotlambda
Copy link
Member

I prefer having to make the dependency on distutils explicit.

@kfollesdal
Copy link
Contributor Author

kfollesdal commented Jul 19, 2024

I prefer having to make the dependency on distutils explicit.

He he ... see we comments on each other PR ;-)

I can understand you point.But think it is better to make setuptools behave as expected. To avoid confusions/problems for people packaging.

If you need to add distutils dependency explicit. How shall people know this? In python world distutils is gone and the way to get backward compatibility is by installing setuptools. Then people need specific knowledge that in nixpkgs we add distutils explicit.

@kfollesdal kfollesdal force-pushed the kfollesdal/setuptools-distutils branch from c01c298 to 3d498dc Compare July 19, 2024 12:12
@emilazy
Copy link
Member

emilazy commented Jul 19, 2024

As I understand it setuptools really doesn’t want people to use their provided distutils which is for their internal use only. Admittedly, exposing it like they do seems contrary to that, but we should probably do a sweep like #326513 to ensure we get rid of this ecosystem wart sooner rather than later.

I’m ambivalent between this and #328379. I prefer to source our distutils from setuptools to ensure that the versions match, but I’m also sympathetic to having an explicit dependency to flag up legacy code, especially in the context of trying to do a treewide sweep. My ideal would probably be to have a separate package that does the ln -s trick this one does to combine what I see as the virtues of the two approaches. On the other hand, this PR makes our behaviour in line with pretty much every other distribution of Python 3.12 out there, which has its own merits.

@emilazy
Copy link
Member

emilazy commented Jul 19, 2024

I forgot that in #326321 we discovered that you can hold setuptools in such a way that it tries to do import distutils. That makes me prefer this. We shouldn’t break setuptools itself and it depends on import distutils in some circumstances.

I’d still like to work on getting rid of all these references at some point though…

@dotlambda
Copy link
Member

I forgot that in #326321 we discovered that you can hold setuptools in such a way that it tries to do import distutils. We shouldn’t break setuptools itself and it depends on import distutils in some circumstances.

Are you worried about it breaking due to incompatible versions? I doubt that would happen as the distutils code probably doesn't change.

On the other hand, this PR makes our behaviour in line with pretty much every other distribution of Python 3.12 out there, which has its own merits.

Not really. There are probably many packages that indirectly depend on setuptools but don't import it because they use e.g. poetry instead. Those shouldn't be able to use distutils either.

@emilazy
Copy link
Member

emilazy commented Jul 19, 2024

Are you worried about it breaking due to incompatible versions? I doubt that would happen as the distutils code probably doesn't change.

I dunno, there’s a whole bunch of commits in the repository you were using that postdate 3.12’s release.

Fair point about the indirect dependencies thing.

@kfollesdal
Copy link
Contributor Author

I forgot that in #326321 we discovered that you can hold setuptools in such a way that it tries to do import distutils. We shouldn’t break setuptools itself and it depends on import distutils in some circumstances.

Are you worried about it breaking due to incompatible versions? I doubt that would happen as the distutils code probably doesn't change.

On the other hand, this PR makes our behaviour in line with pretty much every other distribution of Python 3.12 out there, which has its own merits.

Not really. There are probably many packages that indirectly depend on setuptools but don't import it because they use e.g. poetry instead. Those shouldn't be able to use distutils either.

But that is the behavior in a normal python setup. If you have setuptools installed you can also import distutils.

@dotlambda
Copy link
Member

Not really. There are probably many packages that indirectly depend on setuptools but don't import it because they use e.g. poetry instead. Those shouldn't be able to use distutils either.

But that is the behavior in a normal python setup. If you have setuptools installed you can also import distutils.

No, only after you import setuptools.

@kfollesdal
Copy link
Contributor Author

kfollesdal commented Jul 19, 2024

@dotlambda try to make a normal plain vanilla python venv.

  1. Then try to import distutils, you get ModuleNotFoundError: No module named 'distutils'
  2. pip install setuptools
  3. Now import distutils works without importing setuptools

@emilazy
Copy link
Member

emilazy commented Jul 19, 2024

I think I lean towards @dotlambda’s updated PR at this point, but I’m wondering if anyone has any idea how we could just make the imports work as intended? Could we just make import setuptools add distutils to the Python path? The thing is that as long as setuptools has that internal code that does import distutils like we saw in python-ldap, we’re either going to have to keep a distutils package around even after the ecosystem moves completely on, make the imports work, or patch setuptools.

@pyrox0
Copy link
Member

pyrox0 commented Jul 19, 2024

Instead of patching setuptools and requiring a gigantic rebuild of packages that do not need the changes, instead, why not make a patch for thrift? I've submitted one at apache/thrift#3007 , feel free to use the commit there as a patch. I've confirmed that it applies cleanly to the v0.20.0 tag, though it may require using fetchFromGitHub instead of fetchPypi to apply properly.

@kfollesdal
Copy link
Contributor Author

Instead of patching setuptools and requiring a gigantic rebuild of packages that do not need the changes, instead, why not make a patch for thrift? I've submitted one at apache/thrift#3007 , feel free to use the commit there as a patch. I've confirmed that it applies cleanly to the v0.20.0 tag, though it may require using fetchFromGitHub instead of fetchPypi to apply properly.

Thanks 😊, that is also a possibility, have also an earlier PR where I just fix thrift, #328246

But there are multiple packages that loose python 3.12 support for sam reason. So why not fix nixpkgs setuptools so it behave as vanilla setuptools?

@kfollesdal kfollesdal requested a review from fabaff July 22, 2024 10:43
kfollesdal added a commit to kfollesdal/nixpkgs that referenced this pull request Jul 22, 2024
… at the moment

Depends on thrift that do not support python 3.12 at the moment.

See PR NixOS#328415
setuptools have a trick/hack that make distutils avaliable for import.
This do not work in nix build phase. This commit fix this.
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 10, 2024
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/missing-setuptools-dist-wheel-to-build-a-python-package/55780/1

@mweinelt
Copy link
Member

mweinelt commented Nov 9, 2024

Use distutils instead of fiddling with setuptools.

@mweinelt
Copy link
Member

mweinelt commented Nov 9, 2024

Was apparently fixed in 3a30db4

@mweinelt mweinelt closed this Nov 9, 2024
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.

7 participants