closes bpo-44751: Move crypt.h include from public header to _cryptmodule by geofft · Pull Request #27394 · python/cpython

geofft · 2021-07-27T21:32:13Z

https://bugs.python.org/issue44751

Automerge-Triggered-By: GH:benjaminp

benjaminp · 2021-07-27T21:48:18Z


 #include <sys/types.h>
+#ifdef HAVE_CRYPT_H
+#include <crypt.h>


Do we need to #define _GNU_SOURCE here?

I think no because it's defined by pyconfig.h which is included by Python.h. I'm going to try a build on my Linux box to confirm.

I compiled Python 3.7 on my Debian 9 box with this patch (because that was easiest), and crypt.crypt() works without segfaulting and links crypt_r and there are no warnings in the build log. From inspection, pyconfig.h does define _GNU_SOURCE and does get pulled in by Python.h.

Okay, great.

miss-islington · 2021-07-27T22:33:16Z

@geofft: Status check is done, and it's a success ❌ .

miss-islington · 2021-07-27T22:37:20Z

@geofft: Status check is done, and it's a success ✅ .

miss-islington · 2021-07-27T22:37:21Z

Sorry, I can't merge this PR. Reason: 3 of 5 required status checks have not succeeded: 2 expected..

miss-islington · 2021-07-27T22:58:11Z

@geofft: Status check is done, and it's a success ✅ .

vstinner · 2021-08-04T15:37:18Z

Thank you! I'm not sure why <crypt.h> was in Python.h initially. Likely because it was easier to put it here.

backport python/cpython#27394 to 3.9.6 Android Pycryptodome builds fail due to crypt.h inclusion (not respecting undef HAVE_CRYPT_H)

Arfrever · 2021-09-29T17:43:20Z

Could you backport this fix to at least 3.9 and 3.10 branches?
This is fix for regression which was previously backported to 2.7 and 3.6 branches.

miss-islington · 2021-09-29T20:24:27Z

Thanks @geofft for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

bedevere-bot · 2021-09-29T20:24:38Z

GH-28636 is a backport of this pull request to the 3.10 branch.

…dule (pythonGH-27394) Automerge-Triggered-By: GH:benjaminp (cherry picked from commit 196998e) Co-authored-by: Geoffrey Thomas <geofft@ldpreload.com>

miss-islington · 2021-09-29T20:25:02Z

Thanks @geofft for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

bedevere-bot · 2021-09-29T20:25:11Z

GH-28638 is a backport of this pull request to the 3.9 branch.

…dule (pythonGH-27394) Automerge-Triggered-By: GH:benjaminp (cherry picked from commit 196998e) Co-authored-by: Geoffrey Thomas <geofft@ldpreload.com>

…dule (GH-27394) Automerge-Triggered-By: GH:benjaminp (cherry picked from commit 196998e) Co-authored-by: Geoffrey Thomas <geofft@ldpreload.com>

…dule (GH-27394) (GH-28636) Automerge-Triggered-By: GH:benjaminp (cherry picked from commit 196998e) Co-authored-by: Geoffrey Thomas <geofft@ldpreload.com>

backport python/cpython#27394 to 3.9.6 Android Pycryptodome builds fail due to crypt.h inclusion (not respecting undef HAVE_CRYPT_H)

lazka · 2021-12-28T09:31:55Z

crypt is sadly still needed for packages using python-config:

python3.9-config --libs
 -lcrypt -lpthread -ldl  -lutil -lm -lm

Not sure why python-config leaks all those internals in the first place.

thesamesam · 2021-12-28T09:34:16Z

crypt is sadly still needed for packages using python-config:
python3.9-config --libs
 -lcrypt -lpthread -ldl  -lutil -lm -lm
Not sure why python-config leaks all those internals in the first place.

I think that was fixed separately in #28881 (but not backported). But agreed, I'm not really sure either, and it leads to a lot of applications linking unnecessarily against extra bits.

This whole thing bit us quite hard downstream when changing to libxcrypt and getting build orders right during the migration, as things were requesting it when they didn't actually need it.

lazka · 2021-12-28T09:39:31Z

Ah, thanks. I'm going to backport that then.

Yeah, I guess for static builds it makes sense the way it is, but usually that is hidden behind a "--static" flag.

thesamesam · 2021-12-28T09:53:32Z

Ah, thanks. I'm going to backport that then.

Yeah, I guess for static builds it makes sense the way it is, but usually that is hidden behind a "--static" flag.

FWIW, we've backported it in Gentoo safely and nothing exploded, so it should be okay for MSYS2 too.

What I think we really need/want is for packages to start using pkg-config for this, which does The Right Thing. I've considered making python-config fatal for some tests runs locally to try weed them out, but worried about the huge amount of work.

pkg-config --libs python-3.10 vs pkg-config --libs python-3.10 --static does what you'd expect.

vstinner · 2022-01-07T12:40:39Z

This PR is closed. If there is still a bug, please open a new issue at bugs.python.org ;-)

MarshalX · 2022-01-22T19:03:46Z

Will it be backported to 3.8 and 3.7?

Arfrever · 2022-01-22T22:33:22Z

Those branches accept almost only security fixes, so unfortunately probably no.

vstinner · 2022-01-22T23:02:50Z

Will it be backported to 3.8 and 3.7?

No, you have to backport this change downstream manually (yourself). Python 3.7 and 3.8 no longer accept bugfixes: https://devguide.python.org/#status-of-python-branches

backport python/cpython#27394 to 3.9.6 Android Pycryptodome builds fail due to crypt.h inclusion (not respecting undef HAVE_CRYPT_H)

…dule (pythonGH-27394) Automerge-Triggered-By: GH:benjaminp (cherry picked from commit 196998e)

bpo-44751: Move crypt.h include from public header to _cryptmodule

6890a1a

bedevere-bot added the awaiting review label Jul 27, 2021

the-knights-who-say-ni added the CLA signed label Jul 27, 2021

benjaminp reviewed Jul 27, 2021

View reviewed changes

formatting and concision

5eba578

benjaminp approved these changes Jul 27, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jul 27, 2021

benjaminp added the 🤖 automerge label Jul 27, 2021

benjaminp changed the title ~~bpo-44751: Move crypt.h include from public header to _cryptmodule~~ closes bpo-44751: Move crypt.h include from public header to _cryptmodule Jul 27, 2021

miss-islington merged commit 196998e into python:main Jul 27, 2021

bedevere-bot removed the awaiting merge label Jul 27, 2021

geofft deleted the crypt-public-header branch July 28, 2021 00:29

vstinner added the needs backport to 3.10 only security fixes label Sep 29, 2021

bedevere-bot removed the needs backport to 3.10 only security fixes label Sep 29, 2021

vstinner added the needs backport to 3.9 label Sep 29, 2021

bedevere-bot removed the needs backport to 3.9 label Sep 29, 2021

lazka mentioned this pull request Dec 28, 2021

python: python-config still tries to link to -lcrypt msys2/MSYS2-packages#2759

Closed

gentoo-bot pushed a commit to gentoo/cpython that referenced this pull request May 21, 2024

closes bpo-44751: Move crypt.h include from public header to _cryptmo…

9d792cb

…dule (pythonGH-27394) Automerge-Triggered-By: GH:benjaminp (cherry picked from commit 196998e)

gentoo-bot pushed a commit to gentoo/cpython that referenced this pull request Sep 19, 2024

closes bpo-44751: Move crypt.h include from public header to _cryptmo…

7a1afc9

…dule (pythonGH-27394) Automerge-Triggered-By: GH:benjaminp (cherry picked from commit 196998e)

Uh oh!

Conversation

geofft commented Jul 27, 2021 • edited by miss-islington Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjaminp Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

geofft Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

geofft Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

benjaminp Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Jul 27, 2021

Uh oh!

miss-islington commented Jul 27, 2021

Uh oh!

miss-islington commented Jul 27, 2021

Uh oh!

miss-islington commented Jul 27, 2021

Uh oh!

vstinner commented Aug 4, 2021

Uh oh!

Arfrever commented Sep 29, 2021

Uh oh!

miss-islington commented Sep 29, 2021

Uh oh!

bedevere-bot commented Sep 29, 2021

Uh oh!

miss-islington commented Sep 29, 2021

Uh oh!

bedevere-bot commented Sep 29, 2021

Uh oh!

lazka commented Dec 28, 2021

Uh oh!

thesamesam commented Dec 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lazka commented Dec 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thesamesam commented Dec 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Jan 7, 2022

Uh oh!

MarshalX commented Jan 22, 2022

Uh oh!

Arfrever commented Jan 22, 2022

Uh oh!

vstinner commented Jan 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

geofft commented Jul 27, 2021 •

edited by miss-islington

Loading

thesamesam commented Dec 28, 2021 •

edited

Loading

lazka commented Dec 28, 2021 •

edited

Loading

thesamesam commented Dec 28, 2021 •

edited

Loading