gh-113666: Adding missing UF_ and SF_ flags to module 'stat' #113667

ronaldoussoren · 2024-01-02T21:27:43Z

Add some constants to module 'stat' that are used on macOS.

Issue: Missing UF_ and SF_ flags in Lib/stat.py #113666

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

Add some constants to module 'stat' that are used on macOS.

ronaldoussoren · 2024-01-02T21:30:44Z

There will be an update to this PR, just noticed that there's also a C extension with a definitions.

Lib/stat.py

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

ronaldoussoren · 2024-01-04T12:57:48Z

Something I noticed while looking at stat.py & _stat.c is that the two contain a lot of the same definitions.

IMHO it would be better to strip duplicates (DRY and all), and possibly just drop stat.py (e.g. rename the _stat extension module to stat).

I also don't particularly like defining names that aren't relevant for the current platform (for example S_IFDOOR which AFAIK is only relevant for Solaris). Changing that is something for a different PR though, this would change the interface for stat and requires the usual deprecation cycle before removing.

…nto pythongh-113666

ronaldoussoren · 2024-01-04T16:47:20Z

I've updated the PR:

Fixed typos noted by @serhiy-storchaka
Added the new constants to _stat.c as well
Added a test case that tests the values on macOS (this is a bit overkill, but makes it easier to validate the changes)

I still don't like having the definitions in two places and exporting definitions not relevant for the current platform (e.g. S_IFDOOR on anything that isn't Solaris), I'll file a separate issue about those.

serhiy-storchaka · 2024-01-04T18:53:38Z

Doc/library/stat.rst

+
+   .. versionadded: 3.13
+
+.. data:: UF_SETTABLE


Suggested change

.. data:: UF_SETTABLE

.. data:: SF_SETTABLE

serhiy-storchaka · 2024-01-04T18:59:29Z

I thought about adding some general tests, like testing that UF_SETTABLE, SF_SETTABLE and SF_SYNTHETIC do not intersect, SF_SUPPORTED is a subset of SF_SETTABLE, all UF_* flags are in UF_SETTABLE, and all SF_* flags are in SF_SETTABLE or SF_SYNTHETIC, etc. But your tests are more strict.

I searched for SF_SETTABLE and found it in the xnu sources (https://opensource.apple.com/source/xnu/xnu-2050.18.24/bsd/sys/stat.h), where it's definition is different. Also, there are no SF_SUPPORTED and SF_SYNTHETIC there. Does it matter?

Some flags are defined in stat.py and _stat.c because we want to provide some values even if the platform do not support them. But values from _stat.c override default values in stat.py. If meta-flags are only provided on macOS, I think that there is no need to define default values for them in stat.py.

ronaldoussoren · 2024-01-04T20:15:04Z

I thought about adding some general tests, like testing that UF_SETTABLE, SF_SETTABLE and SF_SYNTHETIC do not intersect, SF_SUPPORTED is a subset of SF_SETTABLE, all UF_* flags are in UF_SETTABLE, and all SF_* flags are in SF_SETTABLE or SF_SYNTHETIC, etc. But your tests are more strict.

My tests are basically written by copying the #define-s from Apple headers (macOS 14.2 SDK) and transforming those to test assertions.

I searched for SF_SETTABLE and found it in the xnu sources (https://opensource.apple.com/source/xnu/xnu-2050.18.24/bsd/sys/stat.h), where it's definition is different. Also, there are no SF_SUPPORTED and SF_SYNTHETIC there. Does it matter?

Those definitions are old, I'm not entirely sure when SF_SYNTHETIC was introduced, but likely when the APFS filesystem was added (10.13), or when they added the system volume and firm links (10.15)

See https://opensource.apple.com/source/xnu/xnu-7195.81.3/bsd/sys/stat.h.auto.html for a more recent copy.

The difference between the older and newer versions shouldn't be a problem, all bits that are set in the older SF_SETTABLE but not in the newer have no assigned meaning in the older release.

Some flags are defined in stat.py and _stat.c because we want to provide some values even if the platform do not support them. But values from _stat.c override default values in stat.py. If meta-flags are only provided on macOS, I think that there is no need to define default values for them in stat.py.

I'll do some more research, google seems to indicate that SF_SETTABLE and SU_SETTABLE are available in at least glibc as well (with the older definition you found), in which case:

Add SU_SETTABLE and SF_SETTABLE to stat.py with the Linux definition, and remove the "if darwin" block
Add SF_SUPPORTED and SF_SYNTHETIC only in stat.c (and override SF_SETTABLE there for macOS by way of using the system definition).

1. SF_SETTABLE and UF_SETTABLE are available on BSD as well Add generic definitions to stat.py and fallback defines to _stat.c (the latter matching how we handle other generic flags) 2. Older versions of macOS do not have SF_SUPPORTED and SF_SYNTHETIC _stat.c defines those for all macOS platforms, and redefines SF_SETTABLE to match recent macOS versions. I did this primarily to end up with an explainable situation: our installers are build on recent versions of macOS and can be used on older versions. The implementation in this changeset ensures we end up with the same definitions regardless of how the binary was build (old SDK or new SDK and old deployment target)

ronaldoussoren · 2024-01-07T10:03:38Z

I didn't find UF_SETTABLE and SF_SETTABLE on Linux, but did find them on FreeBSD. The PR therefore handles them like other generic flags (always available).

A slight complication: As you noticed older versions of macOS have a definition of SF_SETTABLE that doesn't match the definition in the current SDK, and "older" includes at least macOS 10.9 which is the oldest version of the OS targeted by our installers.

To end up with an interface that's easily explainable I've chooses to redefine SF_SETTABLE on those older macOS versions, and define SF_SUPPORTED and SF_SYNTHETIC there as well. That way we can describe those as being available on macOS, without having to explain that availability and values depend on how the installation was build (old SDK vs. new SDK). This should affect usage of these attributes because the extra bits set in the older definition of SF_SETTABLE do not have a defined meaning on those older releases of macOS.

Lib/test/test_stat.py

serhiy-storchaka · 2024-01-07T10:14:52Z

Modules/_stat.c

@@ -579,6 +604,7 @@ stat_exec(PyObject *module)
    ADD_INT_MACRO(module, UF_TRACKED);
    ADD_INT_MACRO(module, UF_DATAVAULT);
    ADD_INT_MACRO(module, UF_HIDDEN);
+    ADD_INT_MACRO(module, SF_SETTABLE);


#ifdef SF_SETTABLE

Modules/_stat.c

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…nto pythongh-113666

pythongh-113666: Adding missing UF_ and SF_ flags to module 'stat'

e5b310b

Add some constants to module 'stat' that are used on macOS.

ronaldoussoren added the OS-mac label Jan 2, 2024

ronaldoussoren self-assigned this Jan 2, 2024

bedevere-app bot added the awaiting core review label Jan 2, 2024

bedevere-app bot mentioned this pull request Jan 2, 2024

Missing UF_ and SF_ flags in Lib/stat.py #113666

Open

ronaldoussoren added the DO-NOT-MERGE label Jan 2, 2024

serhiy-storchaka reviewed Jan 2, 2024

View reviewed changes

Lib/stat.py Outdated Show resolved Hide resolved

Lib/stat.py Outdated Show resolved Hide resolved

Lib/stat.py Outdated Show resolved Hide resolved

Update Lib/stat.py

a577ec8

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

ronaldoussoren added 5 commits January 4, 2024 17:38

Add test that tests values of constants on macOS

8d1f532

Fix definitions

36af895

Add new definitions to _stat.c as well

d00e54f

Merge branch 'pythongh-113666' of github.com:ronaldoussoren/cpython i 1�7

24f9045

…nto pythongh-113666

Fix typo

b68cf31

ronaldoussoren removed the DO-NOT-MERGE label Jan 4, 2024

ronaldoussoren added 2 commits January 4, 2024 19:10

Add new flags to the generic tests as well.

ab7017b

Fix whitespace at end of line

b63cd7e

serhiy-storchaka reviewed Jan 4, 2024

View reviewed changes

ronaldoussoren added 2 commits January 7, 2024 10:30

Merge branch 'main' into pythongh-113666

037863e

serhiy-storchaka reviewed Jan 7, 2024

View reviewed changes

ronaldoussoren and others added 4 commits January 7, 2024 11:19

Fix typo

97ff320

Update Lib/test/test_stat.py

97af986

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

fix ci errors

bae3f7e

Merge branch 'pythongh-113666' of github.com:ronaldoussoren/cpython i 1�7

5cd492d

…nto pythongh-113666

gh-113666: Adding missing UF_ and SF_ flags to module 'stat' #113667

gh-113666: Adding missing UF_ and SF_ flags to module 'stat' #113667

ronaldoussoren commented Jan 2, 2024 •

edited by github-actions bot

ronaldoussoren commented Jan 2, 2024

ronaldoussoren commented Jan 4, 2024

ronaldoussoren commented Jan 4, 2024

serhiy-storchaka Jan 4, 2024

serhiy-storchaka commented Jan 4, 2024

ronaldoussoren commented Jan 4, 2024

ronaldoussoren commented Jan 7, 2024

serhiy-storchaka Jan 7, 2024

gh-113666: Adding missing UF_ and SF_ flags to module 'stat' #113667

Are you sure you want to change the base?

gh-113666: Adding missing UF_ and SF_ flags to module 'stat' #113667

Conversation

ronaldoussoren commented Jan 2, 2024 • edited by github-actions bot

ronaldoussoren commented Jan 2, 2024

ronaldoussoren commented Jan 4, 2024

ronaldoussoren commented Jan 4, 2024

serhiy-storchaka Jan 4, 2024

Choose a reason for hiding this comment

serhiy-storchaka commented Jan 4, 2024

ronaldoussoren commented Jan 4, 2024

ronaldoussoren commented Jan 7, 2024

serhiy-storchaka Jan 7, 2024

Choose a reason for hiding this comment

ronaldoussoren commented Jan 2, 2024 •

edited by github-actions bot