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

Emit crate_root with forward slashes on Windows#200

Merged
dfreese merged 4 commits into
google:masterfrom
rickwebiii:windows
Aug 21, 2020
Merged

Emit crate_root with forward slashes on Windows#200
dfreese merged 4 commits into
google:masterfrom
rickwebiii:windows

Conversation

@rickwebiii

Copy link
Copy Markdown
Contributor

Windows was emitting bazel paths with back slashes for crate_entry attrs because we call path.to_string_lossy. Fix that.

@google-cla

google-cla Bot commented Aug 19, 2020

Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

Comment thread impl/src/planning.rs Outdated
package_root_path_str = package_root_path_str.split_off(2);
}

// Bazel wants forward slashes, but some operating systems don't use '/' as the path

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe a better fix for this would be to change line 750 to not turn the path into a string, call

let package_root_path = target.src_path
  .strip_prefix(&package_root_path())
  .components()
  .map(|c| { c.as_os_str().to_string().expect(format!("Fatal: {:?} has non-UTF-8 characters in it.", c.as_os_str()) })
  .collect::<Vec<String>>()
  .join("/")

This

  1. Obviates the TODO
  2. Obviates 769-771, as components skips . and path separators
  3. Calling to_string_lossy on Paths (which are internally sequences of things built on OsString) because you want to write them in a config file so they can be reliably read by another tool is not generally correct. While unconventional, it's legal in Linux to have non-UTF-8 filenames. If we encounter one, it's better to fail fast in this tool rather than letting than rustc failing with file not found on a path name in the config file that might be visually indistinguishable from the actual path on disk. Not that Bazel can even do well with all legal paths containing only UTF-8 either... Spaces anyone?

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.

I generally like the approach. Are you able to to return an error from the function instead of panicking?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We'd have to change the Result type of this function, as the Error is no longer io::Error, but it's the standard "crate-level Error with a bunch of Variants for all the errors this crate can encounter with a bunch of impl From trait pattern."

I'll have to do something more clever than collect now that I have Vec<Result<String, Error>> rather than Vec coming out of the map statement.

@rickwebiii

Copy link
Copy Markdown
Contributor Author

@googlebot I signed it!

@dfreese dfreese left a comment

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. A small comment in the implementation. Could you also add a unit test to cover the case you were running into?

Then I'm good with this.

Comment thread impl/src/planning.rs Outdated
@dfreese dfreese merged commit 8b4c6b9 into google:master Aug 21, 2020
@dfreese

dfreese commented Aug 21, 2020

Copy link
Copy Markdown
Contributor

This is in v0.3.9.

samschlegel pushed a commit to discord/cargo-raze that referenced this pull request Aug 26, 2020
Adds an additional error for non-unicode paths, which was previously a TODO in terms of handling.  google#178 provided a similar solution.
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