Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-108314: Add PyDict_ContainsString() function#108323

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
vstinner merged 3 commits intopython:mainfromvstinner:contains_string
Aug 24, 2023

Conversation

vstinner
Copy link
Member

@vstinnervstinner commentedAug 22, 2023•
edited by github-actionsbot
Loading

Use PyDict_ContainsString() in pylifecycle.c and pythonrun.c.

📚 Documentation preview 📚:https://cpython-previews--108323.org.readthedocs.build/

Copy link
Member

@methanemethane left a comment

Choose a reason for hiding this comment

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

LGTM except one minor comment.

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

CPython itself does not need this function, because_Py_ID() is used everywhere. But for third-party code it should come in handy.

@@ -73,7 +82,7 @@ Dictionary Objects
.. index:: single: PyUnicode_FromString()

Insert *val* into the dictionary *p* using *key* as a key. *key* should
be a :c:expr:`const char*`. The key object is created using
be a :c:expr:`const char*` UTF-8 encoded bytes string. The key object is created using

Choose a reason for hiding this comment

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

It would be nice to backport such docs changes. Could you create a separate PR for this to make backporting easier? There may be other functions withconst char * parameters without explicitly specified encoding.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Once this PR is merged, I will extract the doc change into a separated PR to backport these doc changes.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I created PR#108448 for the doc changes.

@encukou
Copy link
Member

encukou commentedAug 23, 2023•
edited
Loading

As in#100100, I still don't think this function is worth it. Not every two-liner (well, five-liner in C) needs to be exposed as API.

@serhiy-storchaka
Copy link
Member

I think it is worth it, because attractive alternativesPyDict_GetItemString() andPyMapping_HasKeyString() have flaws.

erlend-aasland reacted with thumbs up emoji

@encukou
Copy link
Member

Can we at least not add to the stable ABI, at least until we have a clearer idea of where the C API is going?

* The new function is not part of the limited C API.* Use PyDict_ContainsString() in pylifecycle.c and pythonrun.c.
@vstinner
Copy link
MemberAuthor

I updated my PR:

  • Don't add the function to the limited C API
  • Fix indentation
  • Add test on invalid UTF-8 string
  • PR rebased on the main branch

@vstinner
Copy link
MemberAuthor

@serhiy-storchaka:

CPython itself does not need this function, because _Py_ID() is used everywhere. But for third-party code it should come in handy.

Sadly yes, CPython has a bias: because CPython has nice tools like Argument Clinic and _Py_ID(), we are less exposed to the annoyance/limitation of the C API.

I hesitated to just use _Py_ID() in the 3 functions that I modified to use PyDict_ContainsString(), but these code paths are not performance critical and I was surprised that there was a "gap" in the API.

I was trying to replace _PyDict_GetItemStringWithError() with PyDict_GetItemStringRef() and I saw that PyDict_GetItemStringRef() also has its annoyance for such code path, since it requires adding Py_DECREF().

@vstinner
Copy link
MemberAuthor

Removing private functions from the public C API (issue#106320) is a way to discover gaps in the public C API. I also think that we should try to restrict ourselves to the public C API (or even the limited C API) in some/most C extensions of the stdlib to discover such issues, and complete the C API if needed.

Private functions like _PyImport_GetModuleAttr() and _PyImport_GetModuleAttrString() are easy to reimplement: it's just PyImport_ImportModule() + PyObject_GetAttr() (and + PyUnicode_FromString() for the String variant), but it's annoying to maintain your own implementation. Where is the limit between "useful recipe of 5-10 lines" and "a public documented and tested API"? I suppose that it should be discussed on a case by case basis.

@vstinner
Copy link
MemberAuthor

test_ssl failed on macOS with:

test_preauth_data_to_tls_client (test.test_ssl.TestPreHandshakeClose.test_preauth_data_to_tls_client) ... skipped "Could not recreate conditions on darwin: err=OSError(22, 'Invalid argument')"Warning -- threading_cleanup() failed to cleanup 0 threads (count: 0, dangling: 2)Warning -- Dangling thread: <SingleConnectionTestServerThread(preauth_data_to_tls_client, stopped 123145377214464)>Warning -- Dangling thread: <_MainThread(MainThread, started 4495689216)>

@vstinner
Copy link
MemberAuthor

I prepared a PR to add the function to pythoncapi-compat:python/pythoncapi-compat#71

@encukou
Copy link
Member

Where is the limit between "useful recipe of 5-10 lines" and "a public documented and tested API"

If we go withMark's proposal of splitting things into a stable, performant, minimal set of “ABI” functions and an ergonomic C API that calls them, these kinds of helpers should go in the latter only.

But will we go with some variation of that proposal? Hopefully we'll have a better idea in a few months :)

@vstinner
Copy link
MemberAuthor

test_ssl failed on macOS with: Warning -- Dangling thread: <SingleConnectionTestServerThread(preauth_data_to_tls_client, stopped 123145377214464)>

I'm working on a fix for that: PR#108344 (merged) and PR#108370.

@vstinner
Copy link
MemberAuthor

@methane@serhiy-storchaka@encukou: I updated my PR, it's no longer part of the limited C API. So what do you think?

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

@vstinnervstinner merged commit6726626 intopython:mainAug 24, 2023
@vstinnervstinner deleted the contains_string branchAugust 24, 2023 13:59
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure⚠️⚠️⚠️

Hi! The buildbotAMD64 Debian PGO 3.x has failed when building commit6726626.

What do you need to do:

  1. Don't panic.
  2. Checkthe buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/249/builds/6075) and take a look at the build logs.
  4. Check if the failure is related to this commit (6726626) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/249/builds/6075

Failed tests:

  • test_venv

Failed subtests:

  • test_with_pip - test.test_venv.EnsurePipTest.test_with_pip

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

440 tests OK.

10 slowest tests:

  • test_concurrent_futures: 2 min 7 sec
  • test_signal: 1 min 13 sec
  • test.test_multiprocessing_spawn.test_processes: 37.9 sec
  • test.test_multiprocessing_forkserver.test_processes: 37.4 sec
  • test_math: 33.6 sec
  • test_io: 30.7 sec
  • test_imaplib: 30.3 sec
  • test_socket: 29.2 sec
  • test_xmlrpc: 27.6 sec
  • test_subprocess: 25.5 sec

1 test failed:
test_venv

15 tests skipped:
test.test_asyncio.test_windows_events
test.test_asyncio.test_windows_utils test_devpoll test_gdb
test_ioctl test_kqueue test_launcher test_startfile test_tkinter
test_ttk test_winconsoleio test_winreg test_winsound test_wmi
test_zipfile64

1 re-run test:
test_venv

Total duration: 9 min 40 sec

Click to see traceback logs
Traceback (most recent call last):  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line781, intest_with_pipself.do_test_with_pip(False)  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line704, indo_test_with_pipwithself.nicer_error():  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/contextlib.py", line159, in__exit__self.gen.throw(value)  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line773, innicer_errorself.fail(AssertionError:Command '['/tmp/test_python_jb_o9fbp/tmpes26zoc8/bin/python', '-m', 'ensurepip', '--upgrade', '--default-pip']' returned non-zero exit status 1.Traceback (most recent call last):  File"<frozen runpy>", line198, in_run_module_as_main  File"<frozen runpy>", line88, in_run_code  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/ensurepip/__main__.py", line5, in<module>    sys.exit(ensurepip._main())^^^^^^^^^^^^^^^^^  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/ensurepip/__init__.py", line284, in_mainreturn _bootstrap(^^^^^^^^^^^  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/ensurepip/__init__.py", line200, in_bootstrapreturn _run_pip([*args,*_PACKAGE_NAMES], additional_paths)^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/ensurepip/__init__.py", line101, in_run_pipreturn subprocess.run(cmd,check=True).returncode^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/subprocess.py", line571, inrunraise CalledProcessError(retcode, process.args,subprocess.CalledProcessError:Command '['/tmp/test_python_jb_o9fbp/tmpes26zoc8/bin/python', '-W', 'ignore::DeprecationWarning', '-c', '\nimport runpy\nimport sys\nsys.path = [\'/tmp/test_python_jb_o9fbp/tmpl3b71vad/pip-23.2.1-py3-none-any.whl\'] + sys.path\nsys.argv[1:] = [\'install\', \'--no-cache-dir\', \'--no-index\', \'--find-links\', \'/tmp/test_python_jb_o9fbp/tmpl3b71vad\', \'--upgrade\', \'pip\']\nrunpy.run_module("pip", run_name="__main__", alter_sys=True)\n']' returned non-zero exit status 1.Traceback (most recent call last):  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line769, innicer_erroryield  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line705, indo_test_with_pipself.run_with_capture(venv.create,self.env_dir,  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line87, inrun_with_capture    func(*args,**kwargs)  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/venv/__init__.py", line469, increate    builder.create(env_dir)  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/venv/__init__.py", line76, increateself._setup_pip(context)  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/venv/__init__.py", line359, in_setup_pipself._call_new_python(context,'-m','ensurepip','--upgrade',  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/venv/__init__.py", line355, in_call_new_python    subprocess.check_output(args,**kwargs)  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/subprocess.py", line466, incheck_outputreturn run(*popenargs,stdout=PIPE,timeout=timeout,check=True,^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/subprocess.py", line571, inrunraise CalledProcessError(retcode, process.args,subprocess.CalledProcessError:Command '['/tmp/test_python_jb_o9fbp/tmpes26zoc8/bin/python', '-m', 'ensurepip', '--upgrade', '--default-pip']' returned non-zero exit status 1.Traceback (most recent call last):  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line781, intest_with_pipself.do_test_with_pip(False)  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line705, indo_test_with_pipself.run_with_capture(venv.create,self.env_dir,  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line87, inrun_with_capture    func(*args,**kwargs)  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/venv/__init__.py", line469, increate    builder.create(env_dir)  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/venv/__init__.py", line74, increateself.setup_python(context)  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/venv/__init__.py", line301, insetup_python    copier(context.env_exe, path,relative_symlinks_ok=True)  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/venv/__init__.py", line236, insymlink_or_copy    shutil.copyfile(src, dst)  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/shutil.py", line273, incopyfile    _fastcopy_sendfile(fsrc, fdst)  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/shutil.py", line164, in_fastcopy_sendfileraise errfromNone  File"/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/shutil.py", line150, in_fastcopy_sendfile    sent= os.sendfile(outfd, infd, offset, blocksize)^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^OSError:[Errno 28] No space left on device: '/tmp/tmpu_g685c0/bin/python' -> '/tmp/tmpu_g685c0/bin/python3.13'

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@methanemethanemethane left review comments

@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes

@encukouencukouAwaiting requested review from encukou

@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@vstinner@encukou@serhiy-storchaka@bedevere-bot@methane
[8]ページ先頭
©2009-2025 Movatter.jp