search

rust-mobile / cargo-subcommand

Library for implementing cargo subcommands
6 stars 6 forks source link

utils: Normalize slashes in multilevel workspace members on Windows #1

Closed MarijnS95 closed 4 years ago

MarijnS95 commented 4 years ago

Multilevel cargo workspace members are always written with a forward slash, for example:

[workspace]
members = [
    "apps/android-app",
]

On Windows forward slashes are not accepted at all, not even fs::canonicalize() is able to normalize them. Calling that function or trying to read a file from a path containing this string results in the following error:

Error: The filename, directory name, or volume label syntax is
incorrect. (os error 123)

Splitting the member name on a forward slash and constructing a new PathBuf from the parts seems the easiest to get a properly normalized path with the right separator for the platform.

Perhaps this line should be scoped by #[cfg(windows)] but I rather keep it generic. I initally expected PathBuf to have a .extend() method accepting an iterator so that the result from .split() can be shoved right in there, but alas. And folding over the result to recursively .join() it to the path seems too verbose, so .collect seems the simplest solution.

dvc94ch commented 4 years ago

Looks good, thanks!

MarijnS95 commented 4 years ago

@dvc94ch Thanks! I gave this another thought and can't realize a Rust world without standard separator conversion function, but Google-fu fails me. Are you perhaps aware of one?

EDIT: I guess that's why a crate exists for it... Oh joy :slightly_smiling_face:

PrototypeNM1 commented 4 years ago

@MarijnS95 I just submitted a PR which overlapped with this change. You might want to check for regressions as the PR replaced your path separator handling with glob.

MarijnS95 commented 4 years ago

@PrototypeNM1 From a very quick look their glob function uses std::path as well and can hence not parse forward slashes correctly. I suggest to leave the replacement in place (since glob seems to expect a string instead of PathBuf, replace / with std::path::MAIN_SEPARATOR) or at least test on Windows whether your patch does not introduce a regression.

I appreciate you pinging me but I am not on Windows while you seem to be; this should be trivial for you test?

PrototypeNM1 commented 4 years ago

I was testing on the following workspace on Windows...

[workspace]
members = ["crates/*", "crates/bevy_ecs/hecs"]

... so I believe it is working. If you have the motivating case I can pull and run I'd be happy to verify it still works.

MarijnS95 commented 4 years ago

@PrototypeNM1 The motivating case - an example even - is right in the commit and PR description.

The glob crate indeed splits on either separator and that only makes sense; it's not natural to write Windows specific code around glob() with backslashes. That means this is working like it should.