Skip to content

posix.pathconf_names #4571

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

Merged
merged 4 commits into from
Feb 27, 2023
Merged

Conversation

youknowone
Copy link
Member

@youknowone youknowone commented Feb 25, 2023

#4494

I don't know the reason but it seems fixed for macos already.

@youknowone
Copy link
Member Author

@marvinmednick could you review the changes?

Copy link

@marvinmednick marvinmednick 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 see any issues with the code changes and like the cleanup, but can you please also re-enablw the test in extra_tests/snippets/stdlib_os.py -- that was what was failing for me. The specfic test that was failing was:
for option,index in os.pathconf_names.items():
assert os.pathconf('/',index) == os.pathconf('/',option)

The tests that need to be enabled for darwin are around line 511

This test basically checks that the pathconf call using the name as a parameter gives the same value as the call with the numeric option (the key and and value of pathconf_names) (Which should always be the case)

The error the CI test was encountering was OSErro error 22 Invalid argument. I found an macos simluator online and found that I could run os.pathconf('/', x) for all x 0 to 20, but would get the error 22 with anything larger than 20. My hypothesis was that there was some entry in PathconfVar that shouldn't be there for macos (and had a value of > 20) (but that was just a guess since I don't have a mac)

I notice that nanos ran pathconf_names on a mac (#4494 (comment)) and the output that was posted seems like it should be OK (at least no ids > 20 which was my guess) I'd like to see if all those pass the test.

The question will be then what was causing the CI test to fail on darwin (and is it still failing)

Also Is it possible to run the CI tests for darwin with some debugging added? -- right now it fails but its not possible to see which iteration it failed out. Adding some debug logging to the test would help figure out which iteration is/was failing.

@marvinmednick
Copy link

marvinmednick commented Feb 26, 2023

FYI I was able to get on the macos simulator again and tried the values listed in #4494 (comment)

{'PC_LINK_MAX': 1, 'PC_MAX_CANON': 2, 'PC_MAX_INPUT': 3, 'PC_NAME_MAX': 4, 'PC_PATH_MAX': 5, 'PC_PIPE_BUF': 6, 'PC_CHOWN_RESTRICTED': 7, 'PC_NO_TRUNC': 8, 'PC_VDISABLE': 9}

and they all worked. I did note that if I did os.pathconf_names in python3 ( I got a longer list than above:
{‘PC_REC_INCR_XFER_SIZE': 14,
'PC_MAX_INPUT': 2,
'PC ASYNC_I0’: 10,
'PC_CHOWN_RESTRICTED’: 6,
'PC_ALLOC_SIZE_MIN': 18,
‘PC_FILESIZEBITS’: 13,
‘PC_PRI0_IO’: 11,
'PC_MAX_CANON’: 1,
'PC_VDISABLE’: 8,
'PC_RECV_MIN_XFER_SIZE: 16,
'PC_PIPE_BUF’: 5,
'PC_SOCK_MAXBUF’: 12,
'PC_LlNK_MAX’: 0,
'PC_NAME_MAX: 3,
'PC_NO_TRUNC’ : 7,
‘PC_SYMLINK_MAX’: 19,
'PC_REC_XFER_ALIGN': 17,
‘PC_PATH_MAX’: 4,
'PC_REC_MAX_XFER_SIZE’: 15,
‘PC_SYNC_IO’: 9}

It would be good to see the output from os.pathconf_names from CPython running on actual mac to see what it reports.
(given this info was from on a simulator (PearOS) all this may info may not be valid.)

@youknowone
Copy link
Member Author

Ah, ok. The error came from pathconf, not from pathconf_names

@youknowone
Copy link
Member Author

You are right. We need more investigation about its difference.

CPython result:

{'PC_ALLOC_SIZE_MIN': 16,
 'PC_ASYNC_IO': 17,
 'PC_CHOWN_RESTRICTED': 7,
 'PC_FILESIZEBITS': 18,
 'PC_LINK_MAX': 1,
 'PC_MAX_CANON': 2,
 'PC_MAX_INPUT': 3,
 'PC_MIN_HOLE_SIZE': 27,
 'PC_NAME_MAX': 4,
 'PC_NO_TRUNC': 8,
 'PC_PATH_MAX': 5,
 'PC_PIPE_BUF': 6,
 'PC_PRIO_IO': 19,
 'PC_REC_INCR_XFER_SIZE': 20,
 'PC_REC_MAX_XFER_SIZE': 21,
 'PC_REC_MIN_XFER_SIZE': 22,
 'PC_REC_XFER_ALIGN': 23,
 'PC_SYMLINK_MAX': 24,
 'PC_SYNC_IO': 25,
 'PC_VDISABLE': 9}

RustPython result:

{'PC_CHOWN_RESTRICTED': 7,
 'PC_LINK_MAX': 1,
 'PC_MAX_CANON': 2,
 'PC_MAX_INPUT': 3,
 'PC_NAME_MAX': 4,
 'PC_NO_TRUNC': 8,
 'PC_PATH_MAX': 5,
 'PC_PIPE_BUF': 6,
 'PC_VDISABLE': 9}

@youknowone youknowone merged commit 351d464 into RustPython:main Feb 27, 2023
@youknowone youknowone deleted the posix-pathconf_names branch February 27, 2023 06:06
@marvinmednick
Copy link

So I think there are two separate issues --

  1. What was causing pathconf to fail in the CI test with the 9 items that are available -- as far as I can tell those entries all look valid. It looked like you tried some debug in the test code -- did you identify anything that that caused the error?

  2. (I think this should get a separate ticket) Why the list of parameters available is RustPython is not the same as in Cypython -- I've looked at the code and think I understand what is happening:
    2a) PathConfVar is the enum for the options -- It uses values from libc to define each entry.
    Many of the variations in the enums are conditionally included based on the os and in particular it looks like they are specifically macos -- (I'm not sure of all the avaiable target_os options, but I did find one example in the RustPython code of target_os="macos" which I'm guessing is the one for macs (and therefore excludes these on mac
    2b) DIgging a little deeper into libc, the constants are defined in a number of files based on OS for example
    the constants these values for linux systems appears to be defined in src/unix/linux_like/linux/mod.rs as follows (with line numbers)
    1298 pub const _PC_LINK_MAX: ::c_int = 0;
    1299 pub const _PC_MAX_CANON: ::c_int = 1;
    1300 pub const _PC_MAX_INPUT: ::c_int = 2;
    1301 pub const _PC_NAME_MAX: ::c_int = 3;
    1302 pub const _PC_PATH_MAX: ::c_int = 4;
    1303 pub const _PC_PIPE_BUF: ::c_int = 5;
    1304 pub const _PC_CHOWN_RESTRICTED: ::c_int = 6;
    1305 pub const _PC_NO_TRUNC: ::c_int = 7;
    1306 pub const _PC_VDISABLE: ::c_int = 8;
    1307 pub const _PC_SYNC_IO: ::c_int = 9;
    1308 pub const _PC_ASYNC_IO: ::c_int = 10;
    1309 pub const _PC_PRIO_IO: ::c_int = 11;
    1310 pub const _PC_SOCK_MAXBUF: ::c_int = 12;
    1311 pub const _PC_FILESIZEBITS: ::c_int = 13;
    1312 pub const _PC_REC_INCR_XFER_SIZE: ::c_int = 14;
    1313 pub const _PC_REC_MAX_XFER_SIZE: ::c_int = 15;
    1314 pub const _PC_REC_MIN_XFER_SIZE: ::c_int = 16;
    1315 pub const _PC_REC_XFER_ALIGN: ::c_int = 17;
    1316 pub const _PC_ALLOC_SIZE_MIN: ::c_int = 18;
    1317 pub const _PC_SYMLINK_MAX: ::c_int = 19;
    1318 pub const _PC_2_SYMLINKS: ::c_int = 20;

There is a separate file which appears to be for apples/mac at src/unix/bsd/apple.mod.rs which has the following defined
2741 pub const _PC_LINK_MAX: ::c_int = 1;
2742 pub const _PC_MAX_CANON: ::c_int = 2;
2743 pub const _PC_MAX_INPUT: ::c_int = 3;
2744 pub const _PC_NAME_MAX: ::c_int = 4;
2745 pub const _PC_PATH_MAX: ::c_int = 5;
2746 pub const _PC_PIPE_BUF: ::c_int = 6;
2747 pub const _PC_CHOWN_RESTRICTED: ::c_int = 7;
2748 pub const _PC_NO_TRUNC: ::c_int = 8;
2749 pub const _PC_VDISABLE: ::c_int = 9;

These 9 match the 9 that are in PathconfVar, and so I think whomever created PathconfVar saw that only these 9 were defined in libc so excluded the others.

Since Cpython on a Mac is actually showing the list you found, the right fix is to first to have them added to libc and then update PathconfVar to not exclude those entries.

@youknowone
Copy link
Member Author

That sonuds like contributing to libc macos target will fix the second problem - while i have no idea about the first one.

@marvinmednick
Copy link

marvinmednick commented Feb 28, 2023

OK So I've done a little more investigation -- I was able to get the CI scripts to run on my local branch and so was able to reproduce the errors on the CI macos platform with some debugging information.

I also created some isolated rust and cpython tests for pathconf and was able to run them through CI on linux and macos.
What I'm seeing on macos (via CI/github actions running on the macos) is that cpython ipathconf_names reports the same list as youknowone posted above, but when you try and access several of the 'supported' values pathconf will actually report an error. In particular pathconf index PC_MAX_CANON (2), PC_MAX_INPUT (3) and PC_VDISABLE(9) all report errors under cpython on the macos (which is what we have been seeing as well).

I've created two tests, one in rust and one in python in the following repo: https://github.com/marvinmednick/pathconf-test

I've attached the results for these tests running under Github actions/CI for macos-latest and ubuntu-latest.
For each the rust and cpython results are reporting the same information (which is good)

>>>> REQUEST <<<<<

I don't know if Github CI is running actual mac hardware or if its running on a VM with macos (or if that would make any difference) but it would be good sanity check if someone could run the same tests and post the results on real mac hardware.

My results from running these on Github CI are attached:
Run rust pathconf-test results.txt
Run python3 pathconf-test results.txt

Conclusion

So using cpython as a baseline on macos pathconf_names reports items that aren't actually useable (which seems like a bug -- i.e. either those values shouldn't be included in pathconf_names or there is a bug in retrieving the values)

Based on this I'm going to propose a updated test that handles this particular error and skips the comparison test if the pathconf by index reports an error. That should be a sufficient test and will allow enabling this for unix systems including mac given that cpython behavior on macos

Separately, cleaning up the libc entries and updating PathconfVar is one option to allow the pathconf_names entries to be consistent with Cpython

Right now the values are dependent both on libc and with specific items excluded by #cfg(target_os=xxx) but I'm not sure if there is a better option. The Cpython code for pathconf _names uses #if defined() (e.g. #if defined(_PC_MAX_INPUT) ) to determine which entries to include but I don't know if there is a way in Rust to check at compile time if a variable is defined (e.g. is libc::_PC_MAX_INPUT defined)

(FYI I found the changes in libc which added the current ones for ma and it looks like person who make the change, only added a few values to have something (there wasn't anything previously), but they didn't try to be exhaustive which explains why it isn't complete and doesn't match what should be available)

@marvinmednick
Copy link

I see that you've go workaround already checked in. Given all I know at the time that all looks good. I've found a include file for the for mac osonline that seems to have the definitions, http://newosxbook.com/src.jl?tree=xnu&file=bsd/sys/unistd.h and if so could be that the issue is not a cypthon issue but an issue with os itself and the os imprelmentaion of pathconf (and that would jive with the Rust calls to pathconf giving the same errror results)

/* configurable pathname variables */
#define _PC_LINK_MAX 1
#define _PC_MAX_CANON 2
#define _PC_MAX_INPUT 3
#define _PC_NAME_MAX 4
#define _PC_PATH_MAX 5
#define _PC_PIPE_BUF 6
#define _PC_CHOWN_RESTRICTED 7
#define _PC_NO_TRUNC 8
#define _PC_VDISABLE 9

#if !defined(_POSIX_C_SOURCE) || defined(_DARWIN_C_SOURCE)
#define _PC_NAME_CHARS_MAX 10
#define _PC_CASE_SENSITIVE 11
#define _PC_CASE_PRESERVING 12
#define _PC_EXTENDED_SECURITY_NP 13
#define _PC_AUTH_OPAQUE_NP 14
#endif

#define _PC_2_SYMLINKS 15 /* Symlink supported in directory /
#define _PC_ALLOC_SIZE_MIN 16 /
Minimum storage actually allocated /
#define _PC_ASYNC_IO 17 /
Async I/O [AIO] supported? /
#define _PC_FILESIZEBITS 18 /
# of bits to represent file size /
#define _PC_PRIO_IO 19 /
Priority I/O [PIO] supported? /
#define _PC_REC_INCR_XFER_SIZE 20 /
Recommended increment for next two /
#define _PC_REC_MAX_XFER_SIZE 21 /
Recommended max file transfer size /
#define _PC_REC_MIN_XFER_SIZE 22 /
Recommended min file transfer size /
#define _PC_REC_XFER_ALIGN 23 /
Recommended buffer alignment /
#define _PC_SYMLINK_MAX 24 /
Max # of bytes in symlink name /
#define _PC_SYNC_IO 25 /
Sync I/O [SIO] supported? /
#define _PC_XATTR_SIZE_BITS 26 /
# of bits to represent maximum xattr size /
#define _PC_MIN_HOLE_SIZE 27 /
Recommended minimum hole size for sparse files */

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants