GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Pathconf names by marvinmednick · Pull Request #4508 · RustPython/RustPython

Conversation

@marvinmednick

@marvinmednick marvinmednick commented Feb 16, 2023

Copy link
Copy Markdown

Implementation for os.pathconf_names resolves #4494

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@marvinmednick

marvinmednick commented Feb 16, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@fanninpm

fanninpm commented Feb 16, 2023

Copy link
Copy Markdown
Contributor

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

youknowone reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@youknowone
youknowone force-pushed the pathconf_names branch from f1e488a to 21072f9 Compare February 17, 2023 12:32
@youknowone

youknowone commented Feb 17, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

youknowone
youknowone requested changes Feb 17, 2023
View reviewed changes
Comment thread extra_tests/snippets/stdlib_os.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread vm/src/stdlib/posix.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

@marvinmednick

marvinmednick commented Feb 18, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@marvinmednick

marvinmednick commented Feb 20, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mmednick @youknowone
Implmentation for os.pathconf_names
468f1aa
@youknowone
youknowone force-pushed the pathconf_names branch from ba0da32 to 8b09202 Compare February 21, 2023 10:26
youknowone
youknowone approved these changes Feb 21, 2023
View reviewed changes

@youknowone youknowone left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread vm/src/stdlib/posix.rs
pathconf(PathOrFd::Fd(fd), name, vm)
}

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

@youknowone youknowone Feb 21, 2023

Copy link
Copy Markdown
Member

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 put linux cfg until it is fixed for macos

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@youknowone
posix.pathconf_names only for linux
955347e
@youknowone
youknowone force-pushed the pathconf_names branch from 8b09202 to 955347e Compare February 21, 2023 10:45
@youknowone
youknowone merged commit 746cb04 into RustPython:main Feb 21, 2023
@youknowone

youknowone commented Feb 21, 2023

Copy link
Copy Markdown
Member

Thank you for contributing!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@youknowone youknowone mentioned this pull request Feb 21, 2023
Enhance compatibility of os.pathconf_names #4494
Open
@marvinmednick

marvinmednick commented Feb 21, 2023

Copy link
Copy Markdown
Author

Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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

Reviewers

@youknowone youknowone youknowone approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enhance compatibility of os.pathconf_names

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@marvinmednick @fanninpm @youknowone @mmednick
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.