Skip to content
This repository was archived by the owner on Sep 24, 2025. It is now read-only.

Root path fix for windows #178

Open
jelmansouri wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Root path fix for windows #178

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion impl/src/planning.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,7 +746,9 @@ impl<'planner> CrateSubplanner<'planner> {
.into_owned()
// TODO(acmcarther): Is this even guaranteed to work? I don't think the `display` output
// can be guaranteed....
.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("\\", "/");

@dfreese dfreese Jun 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@jelmansouri jelmansouri Jun 28, 2020 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think we would still endup with src\lib.rs for example instead of src/lib.rs.

@dfreese dfreese Jun 29, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@acmcarther acmcarther Jun 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, CI doesn't currently run on windows. Travis might have some support for running tests on windows though.

@jelmansouri jelmansouri Jul 1, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@jelmansouri jelmansouri Jul 15, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Can this be accepted, if no CI work is planned please.


// Some crates have a weird prefix, trim that.
if package_root_path_str.starts_with("./") {
Expand Down