Fix //test/rustfmt/... on Windows by minor-fixes · Pull Request #872 · bazelbuild/rules_rust

minor-fixes · 2021-07-31T06:55:57Z

This change aims to fix //test/rustfmt/... tests under Windows.

The root cause of test failures is, generally speaking, attempting to locate runfiles with logic that assumes directory-based runfiles, which is not available on Windows. This change attempts to find all files through the Runfiles utility object so that it can work cross-platform, adding helper methods where necessary to facilitate higher-level logic.

Fixes: #619

`test_main.rs` was looking for runfiles by getting a path to the runfiles dir, and then walking the tree looking for files with a particular extension. This does not work where runfiles are manifest-based rather than directory-based, as is the case on Windows. This change fixes the issue by growing a `list_files` method on the `Runfiles` struct that higher-level code can use to enumerate all the runfiles, filtering out the files they're looking for. Since `find_runfiles_dir` was public purely for this one use case, it can become private; likely higher-level code shouldn't be using this function directly, but rather interacting with runfiles through a Runfiles object. Issues: bazelbuild#619

rustfmt_test should be finding its rustfmt binary and config via runfiles, but it is finding them by naively joining the supplied path with its current working directory. This works for directory-based runfiles, but not manifest-based runfiles. This change makes the discovery of rustfmt and the associated config runfiles-aware.

This change passes the files under test to rustfmt with a path decoded by runfiles, allowing it to work with manifest-based runfiles.

google-cla · 2021-07-31T06:56:08Z

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

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-07-31T06:58:40Z

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

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

minor-fixes · 2021-07-31T07:20:18Z

-    };
-    std::fs::metadata(&absolute_path).map(|_| absolute_path)
+pub fn current_dir_name() -> PathBuf {
+    PathBuf::from(std::env::current_dir().unwrap().file_name().unwrap())


General runfiles question:

Runfiles seem to be passed in/specified as e.g. external/rust_windows_x86_64/bin/rustfmt.exe, but sometimes (concretely, when using manifest-based runfiles) they're expected to be resolved with a workspace name prefix (rules_rust/external/rust_windows_x86_64/bin/rustfmt.exe, in this example, with a rules_rust prefix)

Where is that prefix supposed to come from? Is client code/the runfiles library supposed to assume the workspace name, or divine it somehow? This happens to coincide with the basename of the CWD for both runfiles implementations - hence this sketchy function here - but I'd like to better understand how runfiles are supposed to work r.e. this workspace prefix.

This is a good question. I don't think the prefix is expected to be passed anywhere. Bazel uses a launcher to set some environment variables dictating the locations of runfiles related paths. My experience is that bazel suggests paths to be treated as relative paths from the runfiles directory which is also suggested to be a relative path from the binary. I say "suggest" here since Bazel produces outputs this way but it's obviously up to the author to maintain this if the outputs are distributed in some way.

... This very likely doesn't answer your question. Maybe rephrase and I can try to be more direct 😅

cc @meteorcloudy

minor-fixes · 2021-07-31T07:21:59Z

+pub fn from_slash(p: PathBuf) -> PathBuf {
+    let s = p
+        .to_string_lossy()
+        .chars()
+        .map(|c| match c {
+            '/' => std::path::MAIN_SEPARATOR,
+            c => c,
+        })
+        .collect::<String>();
+    PathBuf::from(s)


Essentially https://docs.rs/path-slash/0.1.4/src/path_slash/lib.rs.html#302; I would have also not liked to re-implement a directory walking function. Can this repo have third-party dependencies on these crates?

There are ways to use 3rd party crates but for this particular tool I'd say it's not desirable since it'd require users to load the dependencies before the target can be used. Maybe there's an easy path forward there that I'm not thinking about but my current thought is that this target should remain free of 3rd party deps 😅

UebelAndre

Sorry for the delay, I like the changes here! 😄

From my end there's just one request for a test and I hope I was able to answer some of your questions.

UebelAndre · 2021-07-31T21:02:40Z

+pub fn from_slash(p: PathBuf) -> PathBuf {
+    let s = p
+        .to_string_lossy()
+        .chars()
+        .map(|c| match c {
+            '/' => std::path::MAIN_SEPARATOR,
+            c => c,
+        })
+        .collect::<String>();
+    PathBuf::from(s)


There are ways to use 3rd party crates but for this particular tool I'd say it's not desirable since it'd require users to load the dependencies before the target can be used. Maybe there's an easy path forward there that I'm not thinking about but my current thought is that this target should remain free of 3rd party deps 😅

UebelAndre · 2021-07-31T21:21:54Z

-    };
-    std::fs::metadata(&absolute_path).map(|_| absolute_path)
+pub fn current_dir_name() -> PathBuf {
+    PathBuf::from(std::env::current_dir().unwrap().file_name().unwrap())


This is a good question. I don't think the prefix is expected to be passed anywhere. Bazel uses a launcher to set some environment variables dictating the locations of runfiles related paths. My experience is that bazel suggests paths to be treated as relative paths from the runfiles directory which is also suggested to be a relative path from the binary. I say "suggest" here since Bazel produces outputs this way but it's obviously up to the author to maintain this if the outputs are distributed in some way.

... This very likely doesn't answer your question. Maybe rephrase and I can try to be more direct 😅

UebelAndre · 2021-07-31T21:29:07Z

+    /// Does not return directories.
+    /// The returned path may not be valid. The caller should check the path's
+    /// validity and that the path exists.
+    pub fn list_files(&self) -> Vec<PathBuf> {


Would you be willing to add a test for this? And perhaps add a few more sample text files to //tools/runfiles/data to make the test a bit more interesting?

Hmm I actually would like to see if we could get away with not having this function at all. Other runfiles libraries didn't need it it seems (https://osscs.corp.google.com/bazel/bazel/+/master:tools/cpp/runfiles/runfiles_src.h, https://osscs.corp.google.com/bazel/bazel/+/master:tools/java/runfiles/Runfiles.java, https://osscs.corp.google.com/bazel/bazel/+/master:tools/bash/runfiles/runfiles.bash).

It seems the main motivation is to be able to find rustfmt manifests in the test data. Is there a reason why we cannot hardcode paths to manifests in the test?

Unless a unique binary is compiled for each use of the rustfmt test binary, there isn't really a way to hard code the values. I think it'd be more desirable to continue to use a shared binary and locate manifests in runfiles than to compile unique binaries but if there's a well defined pattern for runfiles libraries then I'd happily respect that. If there is though, I'd question whether or not the implementations for each language could live all in the same place.

UebelAndre · 2021-08-13T00:07:26Z

Hey, friendly ping on this PR 😄

Let me know if I can do something to help!

UebelAndre · 2022-06-10T22:44:17Z

Closing out in favor of #1375

minor-fixes added 3 commits July 30, 2021 22:52

Find test files via runfiles

a824b08

This change passes the files under test to rustfmt with a path decoded by runfiles, allowing it to work with manifest-based runfiles.

google-cla Bot added the cla: no label Jul 31, 2021

Re-enable rustfmt tests on Windows

18a1501

Resolve clippy errors and compile warnings

d011cab

google-cla Bot added cla: yes and removed cla: no labels Jul 31, 2021

Resolve more clippy errors

35cd5d0

minor-fixes commented Jul 31, 2021

View reviewed changes

minor-fixes added 3 commits July 31, 2021 12:16

Also happens to fix rust_analyzer tests

2c95afd

Retrigger checks

1468ee9

Revert last 2 commits

1dd832b

minor-fixes marked this pull request as ready for review July 31, 2021 20:09

UebelAndre requested changes Jul 31, 2021

View reviewed changes

hlopko self-requested a review August 3, 2021 07:17

This was referenced Jun 4, 2022

Refactored the Rustfmt tool to work cross-platform #1375

Merged

Revert changes to //tools/runfiles from #722 #1402

Closed

UebelAndre closed this Jun 10, 2022

UebelAndre mentioned this pull request Jun 10, 2022

Enable rustfmt CI for Windows. #1403

Merged

Conversation

minor-fixes commented Jul 31, 2021

Uh oh!

google-cla Bot commented Jul 31, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

google-cla Bot commented Jul 31, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UebelAndre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UebelAndre commented Aug 13, 2021

Uh oh!

UebelAndre commented Jun 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants