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

Fix for building on Redox #162

Merged
merged 1 commit into from Sep 28, 2016

Conversation

Projects
None yet
2 participants
@jackpot51
Copy link
Contributor

jackpot51 commented Sep 25, 2016

On a platform that does not set cfg(unix) or cfg(windows), the impl blocks modified in this PR are left empty, causing compilation to fail

This fixes compilation by not attempting to implement Encodable or Decodable on Path or PathBuf on platforms without the OsStr and OsString conversion functions.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 27, 2016

@brson posted more on internals but the tl;dr; is that this patch is a little worrying. Why doesn't std have a path module?

@jackpot51

This comment has been minimized.

Copy link
Contributor Author

jackpot51 commented Sep 27, 2016

It absolutely does have a std::path module. What the issue is with this code is that it assumes that either cfg(unix) or cfg(windows) are set, when a non-unix non-windows OS might not set these config values (such as Redox).

A quick fix is to correctly annotate the impl's. A more complete fix would be to have a third function, #[cfg(redox)], that correctly decodes and encodes on Redox

@jackpot51

This comment has been minimized.

Copy link
Contributor Author

jackpot51 commented Sep 27, 2016

@alexcrichton would you prefer a fix that adds #[cfg(redox)] and correctly decodes and encodes paths? If so, I can make a PR pretty quickly

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 27, 2016

Yes if std::path exists then there should be a code path here for that

@jackpot51

This comment has been minimized.

Copy link
Contributor Author

jackpot51 commented Sep 27, 2016

@alexcrichton In my newest commit, I implemented the #[cfg(redox)] functions. b99dac2

In Redox, OsStr and OsString are guaranteed to be UTF-8 slices. As such, to_str().unwrap() is guaranteed to succeed and OsString::from(String) is valid.

@alexcrichton alexcrichton merged commit 6488e2b into rust-lang-deprecated:master Sep 28, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jackpot51

This comment has been minimized.

Copy link
Contributor Author

jackpot51 commented Nov 3, 2016

@alexcrichton Can you update the crate version so I can use 0.3 in manifests on Redox?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.