Skip to content

python312Packages.python-ldap: fix build #326321

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.

Sign up for GitHub

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

Closed
wegank wants to merge 1 commit into from
Closed

python312Packages.python-ldap: fix build #326321

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions pkgs/development/python-modules/python-ldap/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ buildPythonPackage rec {
version = "3.4.4";
pyproject = true;

disabled = pythonOlder "3.6" || pythonAtLeast "3.12"; # requires distutils
disabled = pythonOlder "3.6";

src = fetchFromGitHub {
owner = "python-ldap";
Expand All @@ -35,7 +35,16 @@ buildPythonPackage rec {
hash = "sha256-v1cWoRGxbvvFnHqnwoIfmiQQcxfaA8Bf3+M5bE5PtuU=";
};

build-system = [ setuptools ];
build-system = [
(setuptools.overrideAttrs {
postPatch = ''
substituteInPlace setuptools/_distutils/util.py \
--replace-fail \
"from distutils.util import byte_compile" \
"from setuptools._distutils.util import byte_compile"
'';
})
];
Comment on lines +38 to +47
Copy link
Member

@emilazy emilazy Jul 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we agreed on Matrix that this seems really wrong, but not on exactly what’s going on here or how it should be fixed. If this is required it should at least be in the main setuptools package, I guess.

Copy link
Member Author

@wegank wegank Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe cython/cython#5754 (comment) provides a clear picture of what happened and how the Python 3.12 distutils issue is specific to Nix. Though setuptools/_distutils is and should be setuptools-free, so I have no idea yet for an upstreamable fix.

Copy link
Member

@phaer phaer Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate why this is really wrong?

At first glance it looks like it would only affect the non-propagated build-system of this specific package. Is it about the import of an explicitly internal vendored distutils from setuptools? Or am I missing something obvious?

Copy link
Member

@emilazy emilazy Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this is breaking on something setuptools is doing internally. It’s generating a script that references distutils.util, running it, and hitting an error. Therefore, either python-ldap is somehow holding setuptools in a way that is really wrong and triggering this error (and therefore we should patch python-ldap, not setuptools), or this would be broken in more packages than just python-ldap, so we should fix it in our setuptools package, or in our general Python environment handling if there’s something we’re doing to cause it. Patching setuptools only within python-ldap can’t be the right answer.

And indeed, @wegank’s link confirms to me that it’s an issue with our Python environment. Perhaps we should include this hack in our setuptools package for now until we figure out a better fix? Do we know the exact chain of features/code that cause setuptools to start executing this vendored distutils code path?

Copy link
Member

@emilazy emilazy Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For context, the relevant code: https://github.com/pypa/setuptools/blob/634d81da556bde64279f36348fd94c7f4ad13bfb/setuptools/_distutils/util.py#L406-L446

Copy link
Member Author

@wegank wegank Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first glance it looks like it would only affect the non-propagated build-system of this specific package. Is it about the import of an explicitly internal vendored distutils from setuptools? Or am I missing something obvious?

Setuptools provides a .pth file to make the vendored distutils importable via import distutils at every Python startup, except that the mechanism doesn't work for Nix. As a result, we're making PRs to solve missing distutils issues that don't exist on other distros.

Copy link
Member

@emilazy emilazy Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW it’s probably not entirely a bad thing that it doesn’t work for us, because things really shouldn’t be using distutils anyway! But it is kind of annoying in the meantime.

If we don’t want to deal with it we could just make setuptools propagate a PYTHONPATH that lets distutils be found directly. I presume there’s a reason they did it the way they did, though.

Copy link
Member

@phaer phaer Jul 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explaination. I miss-understood that the fix was wrong per se, my new understanding is that python3.pkgs.setuptools might be a better place to get it's behavior a bit closer to conventional venvs.

If we don’t want to deal with it we could just make setuptools propagate a PYTHONPATH > that lets distutils be found directly. I presume there’s a reason they did it the way > they did, though.

Thef first link @wegank posted seems to imply the reason could be to shadow an (older) distutils from the standard library where available:

This hack (ab)uses the extra_path behavior in Setuptools to install a .pth file with implicit behavior on startup to give higher precedence to the local version of distutils over the version from the standard library.

This doesn't seem like a concern for us if we apply only for python >=3.12 (where there is no distutils in stdlib)


buildInputs = [
openldap
Expand Down