Skip to content
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

Implements 6 os/posix constants missing for macos #4110

Merged
merged 5 commits into from Sep 26, 2022

Conversation

ecpost
Copy link
Contributor

@ecpost ecpost commented Aug 20, 2022

Implements 6 os/posix constants missing for macos:

  • SEEK_DATA and SEEK_HOLE already exist in posix.rs for a few other target_os's, but they are re-exported from libc, which does not currently include them for macos. So the the consts in this PR are hard-coded.
  • O_EVTONLY, O_FSYNC, O_SYMLINK, and O_NOFOLLOW_ANY did not yet exist in posix.rs. Per the CPython docs, these are only available in macOS. 2 of the 4 are not available in libc. For consistency, 4 all are hard-coded in this PR.

I suppose it would be better to send a PR to libc to have these added (and I'm happy to do that as well), rather than hard-coding them here. But I don't know how long it would take for that change (in whatever libc version) to make its way back here. So I did them as constants that can be switched to libc re-exports if/when they're available in the future.

Ref: #1175

@ecpost ecpost mentioned this pull request Aug 20, 2022
@youknowone
Copy link
Member

Thank you! libc is very responsive and they have frequent release schedules. Once you submit a proper patch, it will be available in 2 weeks in high chance.

@youknowone youknowone self-requested a review August 21, 2022 04:07
const O_EVTONLY: i32 = 0x8000;
#[cfg(target_os = "macos")]
#[pyattr]
const O_FSYNC: i32 = 0x80;
Copy link
Member

Choose a reason for hiding this comment

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

const O_FSYNC: i32 = 0x80;
#[cfg(target_os = "macos")]
#[pyattr]
const O_SYMLINK: i32 = 0x200000;
Copy link
Member

Choose a reason for hiding this comment

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

@youknowone
Copy link
Member

libc update: rust-lang/libc#2886

@ecpost
Copy link
Contributor Author

ecpost commented Aug 21, 2022

Thanks for submitting the PR to libc! I saw your reply last night before I went to bed and was going to do that myself in the morning -- I should've let you know last night, sorry.

For the 2 requested changes, did you just want me to go ahead and use libc for the 2 it has already? (I had purposely done them all as constants for consistency. But I can fix those 2. Just confirming that's what you meant.)

@youknowone
Copy link
Member

I want to keep Python-independent things as much as possible outside of RustPython.

To be honest, if you are not planning to work on another issue depending on this PR, I want to make everything referring libc before merging - unless they take too long time.
If you want to merge it immediately, yes, I want to use libc as much as possible. I want to use libc for those 2 values.

@ecpost
Copy link
Contributor Author

ecpost commented Aug 21, 2022

Yeah, that makes sense. Let's leave it open until libc has them all. (No rush on my side. Just wanted to do an easy first PR on the project.)

@youknowone
Copy link
Member

Thank you for understanding! I will notify you when it done.

@youknowone
Copy link
Member

The PR is merged. It will be released in the next version (probably 0.2.133)

@youknowone
Copy link
Member

@ecpost libc is updated! it took a lot longer time than expected.

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.

Thank you!

@youknowone youknowone merged commit c2a5acb into RustPython:main Sep 26, 2022
@ecpost ecpost deleted the macos-consts branch September 26, 2022 16:22
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.

None yet

2 participants