Root path fix for windows #178
Conversation
| .split_off(package_root_path.display().to_string().len() + 1); | ||
| .split_off(package_root_path.display().to_string().len() + 1) | ||
| // Handle Windows case where we end up here with backslashes | ||
| .replace("\\", "/"); |
There was a problem hiding this comment.
This behavior on lines 743-749/751 looks quite a bit like strip_prefix. Could that be used instead? Would that help get rid of the special casing for windows?
There was a problem hiding this comment.
I think we would still endup with src\lib.rs for example instead of src/lib.rs.
There was a problem hiding this comment.
Thanks for the clarification. For my own reference, from std::path::Path:
separated by / on Unix and by either / or \ on Windows
It would be nice to have a test case for this, though I'm not sure if CI even runs on windows. @acmcarther can you confirm.
There was a problem hiding this comment.
Yeah, CI doesn't currently run on windows. Travis might have some support for running tests on windows though.
There was a problem hiding this comment.
I'll be happy to add CI for windows support.
Commit bazelbuild/rules_rust@d2c4b14 just adds it on the bazel rust rules. So all windows support is quite fresh, if we're building through bazel in the CI we need the latest rust rules.
There was a problem hiding this comment.
Can this be accepted, if no CI work is planned please.
|
I don't have a strong preference on this one way or the other, but would like to wait for dfreese@'s LGTM since he provided an initial review. |
| .split_off(package_root_path.display().to_string().len() + 1); | ||
| .split_off(package_root_path.display().to_string().len() + 1) | ||
| // Handle Windows case where we end up here with backslashes | ||
| .replace("\\", "/"); |
There was a problem hiding this comment.
Thanks for the clarification. For my own reference, from std::path::Path:
separated by / on Unix and by either / or \ on Windows
It would be nice to have a test case for this, though I'm not sure if CI even runs on windows. @acmcarther can you confirm.
Adds an additional error for non-unicode paths, which was previously a TODO in terms of handling. #178 provided a similar solution.
Adds an additional error for non-unicode paths, which was previously a TODO in terms of handling. google#178 provided a similar solution.
|
@jelmansouri Hey, would you be able to rebase and fix the conflict here? @dfreese @acmcarther And would you guys be able to do another review here? Perhaps @damienmg would be able to fill in for @acmcarther if he's not reachable. |
|
Is this change still necessary? |
4 participants
No description provided.