Skip to content
This repository has been archived by the owner on Dec 1, 2023. It is now read-only.

Fix for building on Redox #162

Merged
merged 1 commit into from
Sep 28, 2016
Merged

Fix for building on Redox #162

merged 1 commit into from
Sep 28, 2016

Conversation

jackpot51
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Contributor

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

@jackpot51
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
@jackpot51
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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants