-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
posix.pathconf_names #4571
Conversation
@marvinmednick could you review the changes? |
There was a problem hiding this 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.
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: It would be good to see the output from os.pathconf_names from CPython running on actual mac to see what it reports. |
Ah, ok. The error came from pathconf, not from pathconf_names |
9257579
to
fcacdb2
Compare
You are right. We need more investigation about its difference. CPython result:
RustPython result:
|
So I think there are two separate issues --
There is a separate file which appears to be for apples/mac at src/unix/bsd/apple.mod.rs which has the following defined 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. |
That sonuds like contributing to libc macos target will fix the second problem - while i have no idea about the first one. |
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. 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. >>>> 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: ConclusionSo 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) |
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 */ #if !defined(_POSIX_C_SOURCE) || defined(_DARWIN_C_SOURCE) #define _PC_2_SYMLINKS 15 /* Symlink supported in directory / |
#4494
I don't know the reason but it seems fixed for macos already.