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

add macOS mount flags #1690

Merged
merged 1 commit into from
Mar 13, 2020
Merged

add macOS mount flags #1690

merged 1 commit into from
Mar 13, 2020

Conversation

pwang7
Copy link
Contributor

@pwang7 pwang7 commented Mar 12, 2020

#1688 add macOS mount flags according to Apple sys/mount.h

Closes #1688

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@JohnTitor
Copy link
Member

Since I just added bunch of flags, do these flags have to be used?

Because MNT_FORCE is already defined in:

pub const MNT_FORCE: ::c_int = 0x80000;

You can just remove that const from your defined.
Hm, we may be able to move the consts to bsd/mod.rs (I thought they were defined as c_ulonglong)... But it's out of scope, I can follow-up later if you don't.

And you should comment on this PR, not the issue so that we don't have to come and go :)

@JohnTitor
Copy link
Member

Also, you should resolve style failure.

@pwang7
Copy link
Contributor Author

pwang7 commented Mar 13, 2020

Should I move these flags to libc/src/unix/bsd/mod.rs?

@JohnTitor
Copy link
Member

Should I move these flags to libc/src/unix/bsd/mod.rs?

I guess we can but we may need additional investigation so the answer is "if you like" :)
Also, CI will fail because of apple 32-bit targets, I fixed CI on master so could you rebase? And it'd be great if you could squash commits into one after finishing the work.

@pwang7 pwang7 force-pushed the patch-1 branch 2 times, most recently from c696787 to fbd4ccf Compare March 13, 2020 09:14
@pwang7
Copy link
Contributor Author

pwang7 commented Mar 13, 2020

I did rebase and squash, I also moved the flags to libc/src/unix/bsd/mod.rs, but still some errors.

@JohnTitor
Copy link
Member

Thanks!

I also moved the flags to libc/src/unix/bsd/mod.rs, but still some errors.

Heh, that's "we may need additional investigation" point. It seems some BSDs don't have some flags (therefore use of undeclared identifier error occurs). We should move them to under /bsd/apple. And MNT_AUTOMOUNTED should be defined as ulonglong in some BSDs.

@pwang7
Copy link
Contributor Author

pwang7 commented Mar 13, 2020

BTW, should I move MNT_FORCE from src/unix/bsd/mod.rs to src/unix/bsd/apple/mod.rs?

@JohnTitor
Copy link
Member

BTW, should I move MNT_FORCE from src/unix/bsd/mod.rs to src/unix/bsd/apple/mod.rs?

You don't have to.

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

Looks good! (semver failures are unrelated.)

@JohnTitor JohnTitor merged commit da8f35a into rust-lang:master Mar 13, 2020
@JohnTitor
Copy link
Member

Thanks!

@pwang7
Copy link
Contributor Author

pwang7 commented Mar 14, 2020

Thanks @JohnTitor! This is my first commit to Rust~

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.

mount flags for MacOS (BSD)
3 participants