-
Notifications
You must be signed in to change notification settings - Fork 9
Properly feature gate rustix-based functions #75
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
Conversation
|
I've ensured that compilation succeeds, but was unable to run the test suite as I don't have access to a Windows machine at the moment. |
cgwalters
left a comment
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.
Thanks for the patch. It should be easy to add a GHA Windows build test here, I can help.
4f25e44 to
8687bfc
Compare
|
I did #76 - you can cherry pick that commit into here if you want, or we can merge it over red and then verify this fixes it after you rebase. |
19cf4d6 to
1c7c77e
Compare
Since we regressed here recently.
1c7c77e to
c8df0c4
Compare
cgwalters
left a comment
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.
Thanks!
| assert!(td.remove_all_optional(p).unwrap()); | ||
| td.symlink("linkdest", p)?; | ||
| assert!(td.remove_all_optional(p).unwrap()); | ||
| #[cfg(not(windows))] |
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.
Kind of curious why you picked not(windows) vs unix here.
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.
Because symlink is also gated as not(windows). But could be changed to unix. Not sure whether that would change Mac coverage.
cgwalters
left a comment
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.
Marking needs-work for CI
454425b to
1ea3233
Compare
1ea3233 to
c28d722
Compare
|
I have no idea why the test fails. Maybe someone can help debug on Windows? |
|
Yes no worries, you've done a lot of work here. Thanks! |
Until we can debug what the issue might be. It looks to me like the `tempfile` crate has special logic here https://github.com/Stebalien/tempfile/blob/0657fdf24925287e6d5544d9e321a48d7ceaafdc/src/file/imp/windows.rs#L92 But we're just relying on cap-std, and we need to dig to see if there's something similar or it needs changes.
|
Thanks, contributing has been a great experience! |
|
Awesome 😄 And thank you for the patch! |
As described in #74,
cap-std-ext v4.0.2onwards do not build for Windows.This PR fixes the problems introduced in
v4.0.3, where arustix-based API was improperly feature gated.Additionally, the
noxdevconfig forCapStdExtDirExt::walkseems unsupported under Windows so far and has been disabled there.Fixes #74.