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

gh-123275: Support -Xgil=1 and PYTHON_GIL=1 on non-free-threaded builds #123276

Merged
merged 5 commits into from
Sep 5, 2024

Conversation

ZeroIntensity
Copy link
Member

@ZeroIntensity ZeroIntensity commented Aug 23, 2024

…e-123275.DprIrj.rst

Co-authored-by: Kirill Podoprigora <[email protected]>
Doc/using/cmdline.rst Outdated Show resolved Hide resolved
Co-authored-by: Matt Wozniski <[email protected]>
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

I don't have a strong opinion about this one way or the other. This seems fine to me.

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

lgtm

@corona10 corona10 added the needs backport to 3.13 bugs and security fixes label Sep 5, 2024
@corona10 corona10 merged commit 84ad264 into python:main Sep 5, 2024
40 checks passed
@miss-islington-app
Copy link

Thanks @ZeroIntensity for the PR, and @corona10 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 5, 2024
…eaded builds (pythongh-123276)

(cherry picked from commit 84ad264)

Co-authored-by: Peter Bierma <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Sep 5, 2024

GH-123753 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Sep 5, 2024
@ZeroIntensity ZeroIntensity deleted the support-xgil-nonfreethread branch September 5, 2024 23:54
@corona10
Copy link
Member

corona10 commented Sep 6, 2024

@ZeroIntensity I think that we forgot to update test code.

def test_python_gil(self):
cases = [
# (env, opt, expected, msg)
(None, None, 'None', "no options set"),
('0', None, '0', "PYTHON_GIL=0"),
('1', None, '1', "PYTHON_GIL=1"),
('1', '0', '0', "-X gil=0 overrides PYTHON_GIL=1"),
(None, '0', '0', "-X gil=0"),
(None, '1', '1', "-X gil=1"),
]
code = "import sys; print(sys.flags.gil)"
environ = dict(os.environ)
for env, opt, expected, msg in cases:
with self.subTest(msg, env=env, opt=opt):
environ.pop('PYTHON_GIL', None)
if env is not None:
environ['PYTHON_GIL'] = env
extra_args = []
if opt is not None:
extra_args = ['-X', f'gil={opt}']
proc = subprocess.run([sys.executable, *extra_args, '-c', code],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True, env=environ)
self.assertEqual(proc.returncode, 0, proc)
self.assertEqual(proc.stdout.rstrip(), expected)
self.assertEqual(proc.stderr, '')

Can you update it too?

@ZeroIntensity
Copy link
Member Author

Sure, I'll submit a hotfix. Sorry about that!

@corona10
Copy link
Member

corona10 commented Sep 6, 2024

Sure, I'll submit a hotfix. Sorry about that!

No problem, it was my fault since I approved and merged this PR without checking test code.
Thanks for the work!

@ZeroIntensity
Copy link
Member Author

FWIW, that test doesn't actually run on default builds. I'll just add a new test next to it to make sure that it works.

@corona10
Copy link
Member

corona10 commented Sep 6, 2024

FWIW, that test doesn't actually run on default builds. I'll just add a new test next to it to make sure that it works.

Yeah, I know, but we should add the test when we want to support this feature at the default build, that was what I wanted to mean :)

corona10 pushed a commit that referenced this pull request Sep 6, 2024
…readed builds (gh-123276) (gh-123753)

gh-123275: Support `-Xgil=1` and `PYTHON_GIL=1` on non-free-threaded builds (gh-123276)
(cherry picked from commit 84ad264)

Co-authored-by: Peter Bierma <[email protected]>
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.

6 participants