Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Pathconf names #4508

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 2 commits into from
Feb 21, 2023
Merged

Pathconf names #4508

merged 2 commits into from
Feb 21, 2023

Conversation

marvinmednick
Copy link

Implementation for os.pathconf_names resolves #4494

@marvinmednick
Copy link
Author

marvinmednick commented Feb 16, 2023

Let me know if I did something incorrectly -- I see as part my pull requet there are commits authored by others.... The two changes I'm looking to pull in for pathconf_names are d8fe778 and df48266

@fanninpm
Copy link
Contributor

After CI completes, feel free to do a git rebase ... followed by a git push --force.

@youknowone
Copy link
Member

I rebased it. git rebase is very useful tool to send patches.
https://docs.gitlab.com/ee/topics/git/git_rebase.html

extra_tests/snippets/stdlib_os.py Outdated Show resolved Hide resolved
vm/src/stdlib/posix.rs Outdated Show resolved Hide resolved
@marvinmednick
Copy link
Author

I rebased it. git rebase is very useful tool to send patches. https://docs.gitlab.com/ee/topics/git/git_rebase.html

Thanks -- I was looking at trying to do that but it didn't seem to look right as I still ended up with 2 additional changes -- I was thinking my baseline wasn't quite right or I did something incorrectly.

@marvinmednick
Copy link
Author

marvinmednick commented Feb 20, 2023

I'm looking at the error -- I found an online free online macos simlulator and was able to get the error 22 manually trying the command os.pathconf('/',21) but that isn't conclusive. (pathconf worked for indexes 0-20).
I'd like to check the the PathconfVar entries for darwin... but darwin doesn't seem to be a confguration check within PathconfVar

It looks like there are some platforms which may have pathconf indexes that are greater then 20 (e.g. in libc
libc: src/unix/haiku/mod.rs:pub const _PC_REC_MAX_XFER_SIZE: ::c_int = 32;
though on linux the same parameter is index 15.
libc src/unix/linux_like/linux/mod.rs:pub const _PC_REC_MAX_XFER_SIZE: ::c_int = 15;

I suppose this is more a of libc specfic question does anyone know how can I tell which of the libc flavor will be used on macos/darwin? (and therefore which pathconf indexes value will show up in PathconfVar)

Clarifying -
Change is now failing due to test running on darwin (which started running when making the change to the conditional to allow it to run on darwin. It suspect that one of the entries in PathconfVar isn't valid on that platform - the code takes all the variants in enums and converts them to the dict form. The test verifies the the value that one gets when using the name is the same as the value when using the index which should always be true, However the test is failing when it gets an error value as the parameter -- which I belive is coming from the enum.

As a result I suspect there may be a value in PathconVar which isn't valid for pathconf on that platform.... (and causing the error)

The test could be simplified to only check a few manually selected values rather , but if this is actually I think it would be better to fix PathconfVar to have the correct values for the platform.

Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to fix every macOS issue if if you don't have macOS machine. Please fill free to ask help if you have hard trouble to fix.

I think adding it only for linux is a working option now. I made a change about it. We can fix macOS issue later.

Thank you for contributing!

@@ -1881,6 +1883,24 @@ pub mod module {
pathconf(PathOrFd::Fd(fd), name, vm)
}

// TODO: this is expected to be run on macOS as a unix, but somehow not.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put linux cfg until it is fixed for macos

@youknowone youknowone merged commit 746cb04 into RustPython:main Feb 21, 2023
@youknowone
Copy link
Member

Thank you for contributing!

@marvinmednick
Copy link
Author

Thanks!

@marvinmednick marvinmednick deleted the pathconf_names branch February 26, 2023 22:53
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.

Enhance compatibility of os.pathconf_names
4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.