gh-90026: support XATTRs on Cygwin

[philcerf]

As described in #90026 (comment) and following comments, CPython’s code for XATTRs seems to already support Cygwin… mostly.

Apart from the changes made by this PR (simply compiling the relevant code), there are however also some changes needed on the Cygwin side (I guess):

Namely, they don't export XATTR_SIZE_MAX and XATTR_LIST_MAX and so a build of CPython with this commit would fail right now.

I've asked at their mailing list to have them included in their cygwin/limits.h (which I think is the proper place - Linux defines them in linux/limits.h): https://cygwin.com/pipermail/cygwin/2023-May/253750.html

Right now, he newest Version of CPython in Cygwin is 3.9.9... so unless I can get this change backported, they won't be affected by this (well and even if, ... it would be some more "motivation" to set the missing defines, I guess ;-) ).

I could of course also check whether the symbols are defined, as in:

#if defined(HAVE_SYS_XATTR_H)
#  if defined(__linux__) && !defined(__FreeBSD_kernel__) && !defined(__GNU__)
#    define USE_XATTRS
#    include <linux/limits.h>  // Needed for XATTR_SIZE_MAX on musl libc.
#  endif
#  if defined(__CYGWIN__)
#    include <cygwin/limits.h>  // Needed for XATTR_SIZE_MAX and XATTR_LIST_MAX.
#    if defined(XATTR_SIZE_MAX) && defined(XATTR_LIST_MAX)
#      define USE_XATTRS
#    endif
#  endif
#endif

I found that to be a bit ugly and overkill,... but if Python developers like it more... I wouldn’t mind to adapt my commit.

Thanks

• Issue: xattr support missing in os module under cygwin #90026

[bedevere-bot]

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

[cpython-cla-bot]

All commit authors signed the Contributor License Agreement.

[philcerf]

For the records:

I've submitted a patch against Cygwin-newlib at https://sourceware.org/pipermail/newlib/2023/020347.html which - to my best knowledge - should set the (hopefully correct) XATTR_*_MAX values.

[philcerf]

Updated commit also changes the documentation in os.rst to indicate that this is then also available on Cygwin.

What I didn't know (Python experts please help ;-) ) was, whether there is a way how you encode that this will be only available with 3.13 (on Cygwin) and not 3.3 (as on Linux). I've grepped a bit but found no obvious way.

[philcerf]

Just for the records:

The commit that exports the necessary symbols in Cygwin has just been merged.

So future versions of Cygwin’s cygwin/limits.h should provide XATTR_NAME_MAX, XATTR_SIZE_MAX and XATTR_LIST_MAX for CPython to use on that platform.

Would be awesome if this could be reviewed and still make it into 3.13 at least (not sure if a backport would be feasible - guess it’s probably not important enough ^^).

[iritkatriel]

Sorry I don't know anything about Cygwin and I'm working on a Mac.

[philcerf]

Maybe one should also adapt:

cpython/Doc/library/os.rst

Lines 3741 to 3742 in 8571b27

	Linux extended attributes
	~~~~~~~~~~~~~~~~~~~~~~~~~

OTOH, these functions are still "defined" by Linux (FreeBSD and others use different syscalls)... and Cygwin just mimics them. So it may also be justified to let the section named as is.

[philcerf]

Anything I can do to get that reviewed? :-)

[philcerf]

Rebased on master because of changes made meanwhile to posixmodule.c.

But, TBH, I'm not really sure whether it's worth keeping this MR open, as it seems impossible to get even such an extremely simple change reviewed :-(

[willingc]

@philcerf The code looks reasonable to me. I'm not a Cygwin user though.

@encukou Would you be able to give this a look? Thanks. Discourse discussion

[encukou]

I'm fine with adding the #ifdef change -- it's a few lines clearly identified as Cygwin-only.
But I'm less sure about the docs update. It suggests that XATTRs will continue to work on Cygwin, but we have no tests to ensure this.
Would you be OK merging without the docs change?

You could also find wording for the docs that doesn't say this strongly that XATTRs are Linux specific -- but IMO it'd again be better to add that to a dedicated “CPython on Cygwin” page (outside CPython).

I'll post more about process on the Discourse discussion.

[philcerf]

I'd be okay with a commit without any docs changes (@JelleZijlstra would that be also okay for you, since you've proposed adding the minimum Cygwin version).

May make even sense, cause I guess we don't document all other things that work or don't work with Cygwin - and I think that is reasonable as Cygwin is a compatibility layer, so what they support from the API is purely their business.

I had only added it previously, as strictly speaking Cygwin is a POSIX compatibility layer, whereas the API used for the XATTR is Linux specific.

[JelleZijlstra]

I'd be okay with a commit without any docs changes (@JelleZijlstra would that be also okay for you, since you've proposed adding the minimum Cygwin version).

Yes, that's fine with me. If we document it, we should mention the CPython version when the change was made, but Petr's suggestion to omit it from the public docs also makes sense.

(Minor feedback: It's generally better to avoid force pushing. We'll squash merge anyway when we merge the PR, and it's easier to follow the review history if there are no force pushes.)

[philcerf]

No idea why that one test failed.

Edit: Ah, now the tests ran through.

[encukou]

Thank you for the contribution!

[philcerf]

Thanks for merging.

Is it feasible to get this backported? Though it's really not that important, if too much of an effort.

[JelleZijlstra]

I think this counts as a new feature, which we don't backport. However, if you're building from source, you can obviously apply the patch yourself.