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

Enable utimensat for macos target OS #43

Merged
merged 4 commits into from
Aug 13, 2019
Merged

Enable utimensat for macos target OS #43

merged 4 commits into from
Aug 13, 2019

Conversation

kubkon
Copy link

@kubkon kubkon commented Aug 12, 2019

This PR enables utimensat for MacOS target. It's probably not done in the most ergonomic way possible but I'm happy to work on it until it meets the expectations :-) Anyhow, it fixes problems with MacOS target in CraneStation/wasi-common#54.

cc @marmistrz @alexcrichton

Copy link
Owner

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looking at the API it looks like this was a relatively recent addition to OSX, only in 10.13 was the utimensat API added. If that's the case I think we'll want this to retain compatibility with older OSX releases, so could this use dlsym and such to detect when utimensat is available on newer systems and use that, but otherwise fallback to the current implementation?

src/unix/macos.rs Outdated Show resolved Hide resolved
src/unix/macos.rs Outdated Show resolved Hide resolved
@kubkon
Copy link
Author

kubkon commented Aug 12, 2019

Thanks for the PR! Looking at the API it looks like this was a relatively recent addition to OSX, only in 10.13 was the utimensat API added.

Yep, it looks like it.

If that's the case I think we'll want this to retain compatibility with older OSX releases, so could this use dlsym and such to detect when utimensat is available on newer systems and use that, but otherwise fallback to the current implementation?

SGTM! I'll make the necessary adjustments and report back here :-)

src/unix/macos.rs Outdated Show resolved Hide resolved
src/unix/macos.rs Outdated Show resolved Hide resolved
src/unix/macos.rs Outdated Show resolved Hide resolved
src/unix/macos.rs Outdated Show resolved Hide resolved
Changes:
* if utimensat unavailable, fall back to utimes (using lazy loading
trick akin to weak.rs)
* re-defines UTIME_OMIT const on MacOS which, according
to Darwin sources, equals -2
* includes additional MacOS images in Travis for better testing
* adds some msgs to asserts for easier bug tracking in tests
@kubkon
Copy link
Author

kubkon commented Aug 13, 2019

So here's a weird conundrum: the changes pass the tests for MacOS pre 10.13, and for MacOS 10.14 with XCode 11.0. For anything in-between, however, the tests fail at setting the file access time while leaving modified time alone only! I have absolutely no idea what could be the cause of this. I'll try to get my hands on an older version of MacOS and test it out in the VM (I've got only the latest MacOS at hand and everything passes at my end so I cannot reproduce the Travis CI problem). Anyhow, if you've got any ideas/suggestions, please do let me know! This problem's been eating away at my brain for the entire night last night!

We've already got one for the symbol address so no need to make a cache
for that one too.
@alexcrichton
Copy link
Owner

This may just be a bug in OSX from reports like haskell/directory#88, although I can't find much other information on the problem.

I pushed a few commits with small refactorings, but otherwise I think it's fine to remove the other versions of OSX from testing and just test on the latest. This crate doesn't receive all that many changes to it tends to not be worth it to have a comprehensive CI matrix

@kubkon
Copy link
Author

kubkon commented Aug 13, 2019

This may just be a bug in OSX from reports like haskell/directory#88, although I can't find much other information on the problem.

Yeah, it seems like it is! So, I've managed to get my hands on HighSierra running in a VM (via QEMU/KVM on Arch, don't ask me why this crazy setup), and I've managed to replicate the problem 100% even using pure C syscalls. From what I can see, the bug seems to be buried deep with setattrlistat and fsetattrlist syscalls, but no idea where as I can't find the source for either of those. If you have any suggestions here, lemme know and I'll be happy to explore them!

I pushed a few commits with small refactorings, but otherwise I think it's fine to remove the other versions of OSX from testing and just test on the latest. This crate doesn't receive all that many changes to it tends to not be worth it to have a comprehensive CI matrix

Saw them, thanks! Okay dokay then, I'll remove other versions of OSXs and I guess we can go from there.

@alexcrichton alexcrichton merged commit 70fc9d0 into alexcrichton:master Aug 13, 2019
@alexcrichton
Copy link
Owner

👍

@kubkon kubkon deleted the macos-utimensat branch August 13, 2019 17:09
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.

2 participants