gh-127350: Add Py_fopen() function by vstinner · Pull Request #127821 · python/cpython · GitHub

Rename _Py_fopen_obj() to Py_fopen(). The function now also accepts bytes path on Windows.

Remove the private, undocumented, and untested function _Py_fopen_obj().

Issue: _Py_wfopen no longer exported #127350

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

Rename _Py_fopen_obj() to Py_fopen(). The function now also accepts bytes path on Windows. Remove the private, undocumented, and untested function _Py_fopen_obj().

Do we need Py_fclose as well?

Do we need Py_fclose as well?

fclose seems to be enough, we don't need to wrap it(Unless we use CloseHandle to close the file).

@zooba:

Do we need Py_fclose as well?

Py_fopen() adds values to fopen(): accept a Python object, make the file descriptor non-inheritable, raise an exception on error.

Py_fclose() would just call fclose() without any additional value? I would prefer to not add it unless we have to, and call directly fclose(). I asked the reporter if Py_fclose() would be needed.

To be honest, I don't understand well the Windows DLL issue. From what I understood, to use a FILE* in Python DLL, it should be open in the Python DLL. Otherwise, things can go wrong if the C library version is not exactly the same in two DLLs. But I don't know if it's safe to call fopen() / Py_fopen() in DLL A and call fclose() in DLL B.

PyRun_AnyFileExFlags has "closeit" parameter which I always used.
But you are correct. Py_fopen() needs Py_fclose().

Thank you guys for doing all this.

But you are correct. Py_fopen() needs Py_fclose().

Is Py_fclose() needed because of the DLL issue?

Is Py_fclose() needed because of the DLL issue?

Yes. When you call Py_fopen(), you will get a FILE * from whichever C runtime Python is linked to, which isn't necessarily the same as your own application. So you can only pass it back to the same C runtime - it must only be passed into Python APIs, essentially. So regular fclose might crash (or close a different file), and you need Py_fclose() to make sure it goes back to the right runtime.

Ok, I wanted to make sure that I get the rationale correctly.

I added Py_fclose() function and documented that files opened by Py_fopen() must only be closed by Py_fclose().

Your updated documentation seems a bit snarky. The Py_fclose may be needed on any platform depending on your static linking situation.

I see no reason not to just document it as "should be used to close files opened by Py_fopen" and if people manage to use fclose and it works then good for them.

Your updated documentation seems a bit snarky. The Py_fclose may be needed on any platform depending on your static linking situation.

It's not my intent to be snarky. I just tried to explain why calling fclose() directly can cause issues in practice.

I see no reason not to just document it as "should be used to close files opened by Py_fopen" and if people manage to use fclose and it works then good for them.

Ok, let's do that.

Co-authored-by: Steve Dower <steve.dower@microsoft.com>

I created capi-workgroup/decisions#51: "Add public Py_fopen() and Py_fclose() functions, and remove private _Py_fopen_obj() function".

LGTM in general. I would like to see more tests. The support of the path-like protocol is inconsistent.

TODO: Add tests for mode: NULL, non-decodable (non-ASCII) mode, too long mode (e.g. "rt+, ccs=UTF-8").

I added a test for "too long mode" and non-ASCII mode.

Passing NULL does crash. I don't know how to modify the Argument Clinic code to accept NULL.

You can use "z#" to pass arbitrary sequence of bytes or NULL. In Argument Clinic it is expressed as str(zeroes=True, accept={robuffer, str, NoneType}).

Use the NULLABLE macro for path.

If it crashes, just add a comment # CRASHES with specific call as in other tests as indication that we did not miss such case, but cannot test it.

If it crashes, just add a comment # CRASHES with specific call as in other tests as indication that we did not miss such case, but cannot test it.

Ok, I added a comment.

Test also non-ASCII mode.

* Skip TESTFN_UNENCODABLE if it's None. * Remove TESTFN_UNDECODABLE test.

"\xe9" is encoded to b'\xc3\xa9'. Please test also with non-UTF-8 bytes. You may get different error on Windows. Actually, it may depend on the locale.

It's not possible to pass non-UTF-8 bytes, PySys_Audit() decodes the mode from UTF-8 in strict mode:

    if (PySys_Audit("open", "Osi", path, mode, 0) < 0) {
        return NULL;
    }

I don't think that it's worth to "support" non-UTF-8 just for the test, whereas it's rejected anyway by fopen().

You can use "z#" to pass arbitrary sequence of bytes or NULL. In Argument Clinic it is expressed as str(zeroes=True, accept={robuffer, str, NoneType}).

Use the NULLABLE macro for path.

If it crashes, just add a comment # CRASHES with specific call as in other tests as indication that we did not miss such case, but cannot test it.

Yet more test suggestions.

@serhiy-storchaka: I tried to implement all requested tests, but I skipped some of them for practical reasons.

On Windows, the file mode is limited to 10 characters. If it's a problem later, I can allocate memory on the heap and write more complicated code to handle longer modes. In practice, we should be fine with this arbitrary limit.

PR merged, thanks for your reviews!

@serhiy-storchaka: You can write a follow-up PR if you want to extend the code coverage (add more tests).

)

pythongh-127350 : Add Py_fopen() function

773c4d9

Rename _Py_fopen_obj() to Py_fopen(). The function now also accepts bytes path on Windows. Remove the private, undocumented, and untested function _Py_fopen_obj().

vstinner requested review from ericsnowcurrently and kumaraditya303 as code owners December 11, 2024 12:27

bedevere-app Bot mentioned this pull request Dec 11, 2024

_Py_wfopen no longer exported #127350

Closed

bedevere-app Bot added the awaiting core review label Dec 11, 2024

rruuaanng reviewed Dec 12, 2024

View reviewed changes

Comment thread Modules/_ssl.c

Add Py_fclose() function

b80f424

vstinner added 2 commits December 13, 2024 14:37

Merge branch 'main' into py_fopen

14fb249

Replace _Py_fopen_obj() with Py_fopen() in sysmodule.c

7390c5e

Simplify Py_fclose() documentation

26a5a56

Run make clinic

4375566

zooba reviewed Dec 13, 2024

View reviewed changes

Comment thread Doc/c-api/sys.rst Outdated

Update Doc/c-api/sys.rst

a779321

Co-authored-by: Steve Dower <steve.dower@microsoft.com>

zooba approved these changes Dec 16, 2024

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting core review labels Dec 16, 2024

Document Py_fclose() return value

e613374

vstinner mentioned this pull request Dec 16, 2024

Add public Py_fopen() and Py_fclose() functions, and remove private _Py_fopen_obj() function capi-workgroup/decisions#51

Closed

12 tasks

serhiy-storchaka self-requested a review December 31, 2024 15:16

serhiy-storchaka approved these changes Dec 31, 2024

View reviewed changes

kumaraditya303 removed their request for review January 3, 2025 10:17

erlend-aasland reviewed Jan 4, 2025

View reviewed changes

Comment thread Doc/c-api/sys.rst Outdated

Comment thread Doc/c-api/sys.rst Outdated

erlend-aasland reviewed Jan 4, 2025

View reviewed changes

Comment thread Doc/c-api/sys.rst Outdated

vstinner added 9 commits January 6, 2025 11:30

Merge branch 'main' into py_fopen

a908b72

Address some of Serhiy's review

2af4d2b

Address Erlend's review

8e9170e

Add more tests

a8c26b2

Support __fspath__() protocol on Windows

5b52a4e

Test also non-ASCII mode.

Update doc for path-like object

e6a53fe

Keep _Py_fopen_obj() function

1d1e7f4

Fix typo

335c1bc

Fix test on macOS and WASI

8241422

* Skip TESTFN_UNENCODABLE if it's None. * Remove TESTFN_UNDECODABLE test.

vstinner enabled auto-merge (squash) January 6, 2025 11:37

vstinner disabled auto-merge January 6, 2025 11:37

vstinner enabled auto-merge (squash) January 6, 2025 11:38

erlend-aasland approved these changes Jan 6, 2025

View reviewed changes

serhiy-storchaka reviewed Jan 6, 2025

View reviewed changes

vstinner disabled auto-merge January 6, 2025 11:54

vstinner added 3 commits January 6, 2025 13:13

Rewrite the Windows implementation

c91aa82

Use FakePath

0fc6dc1

NULL mode does crash

dda4335

vstinner enabled auto-merge (squash) January 6, 2025 12:36

vstinner merged commit f89e5e2 into python:main Jan 6, 2025

vstinner deleted the py_fopen branch January 6, 2025 12:43

bedevere-app Bot removed the awaiting merge label Jan 6, 2025

srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025

pythongh-127350 : Add Py_fopen() and Py_fclose() functions (python#127821

a6bda81

)

sonVishal mentioned this pull request Jul 1, 2026

Py_fopen and Py_fclose not exported from python3.dll on Windows #152805

Closed

-                                              wmode, Py_ARRAY_LENGTH(wmode));
-                  if (usize == 0) {
-                      PyErr_SetFromWindowsErr(0);
+                      wchar_t wmode[10];

-                                              wmode, Py_ARRAY_LENGTH(wmode));
-                  if (usize == 0) {
-                      PyErr_SetFromWindowsErr(0);
+                      wchar_t wmode[10];

+                      # non-ASCII mode failing with "Invalid argument"
+                      with self.assertRaises(OSError):
+                          _testcapi.py_fopen(__file__, "\xe9")

gh-127350: Add Py_fopen() function by vstinner · Pull Request #127821 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-127350: Add Py_fopen() function - #127821

gh-127350: Add Py_fopen() function#127821vstinner merged 20 commits intopython:mainpython/cpython:mainfrom vstinner:py_fopenvstinner/cpython:py_fopenCopy head branch name to clipboard

Conversation

vstinner commented Dec 11, 2024 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zooba commented Dec 11, 2024

Uh oh!

Uh oh!

rruuaanng commented Dec 12, 2024

Uh oh!

vstinner commented Dec 12, 2024

Uh oh!

PlanetCNC commented Dec 12, 2024

Uh oh!

vstinner commented Dec 12, 2024

Uh oh!

zooba commented Dec 12, 2024

Uh oh!

vstinner commented Dec 13, 2024

Uh oh!

zooba commented Dec 13, 2024

Uh oh!

vstinner commented Dec 13, 2024

Uh oh!

Uh oh!

vstinner commented Dec 16, 2024

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Dec 31, 2024

Choose a reason for hiding this comment

Uh oh!

vstinner Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

vstinner Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jan 6, 2025

Uh oh!

vstinner commented Jan 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gh-127350: Add Py_fopen() function#127821
vstinner merged 20 commits into
python:mainfrom
vstinner:py_fopen

vstinner commented Dec 11, 2024 •
edited by github-actions Bot
Loading

serhiy-storchaka Jan 6, 2025 •
edited
Loading