Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Rust formatting when //:format is called in different subdirectory #327

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

dmeister
Copy link
Contributor

@dmeister dmeister commented Jul 10, 2024

We have been seeing problems when the Rust formatter is called from a subdirectory and not the workspace root directory.

The root cause is that rustfmt appears to look for the file list based on the BUILD_WORKING_DIRECTORY instead of
BUILD_WORKSPACE_DIRECTORY or the current working directory of format.sh

This patch fixes the issue by unsetting the BUILD_WORKING_DIRECTORY if format.sh has BUILD_WORKSAPCE_DIRECTORY is set.

This has been locally tested:

before:
Running in a subdirectory, e.g. b

> bazel run //:format
INFO: Invocation ID: c0ef6738-6805-4ee3-8d5a-ab5a7c393865
INFO: Analyzed target //:format (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //tools:format up-to-date:
  bazel-bin/tools/format.bash
INFO: Elapsed time: 0.351s, Critical Path: 0.03s
INFO: 2 processes: 2 internal.
INFO: Build completed successfully, 2 total actions
INFO: Running command line: bazel-bin/tools/format.bash
684 files left unchanged
Formatted Python in 0m0.016s
Formatted Starlark in 0m0.066s
Formatted Jsonnet in 0m0.144s
Formatted Go in 0m0.067s
Formatted Shell in 0m0.004s
Formatted Protocol Buffer in 0m0.978s
Formatted C++ in 0m0.010s
Formatted YAML in 0m0.019s
Error: file `a/lib.rs` does not exist
Formatted Rust in 0m0.020s
FAILED: A formatter tool exited with code 123

after:

> bazel run //:format
INFO: Invocation ID: 0b2f4cff-24af-474a-aa25-1a32d0ce74a9
INFO: Analyzed target //:format (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //tools:format up-to-date:
  bazel-bin/tools/format.bash
INFO: Elapsed time: 0.380s, Critical Path: 0.03s
INFO: 2 processes: 2 internal.
INFO: Build completed successfully, 2 total actions
INFO: Running command line: bazel-bin/tools/format.bash
684 files left unchanged
Formatted Python in 0m0.017s
Formatted Starlark in 0m0.066s
Formatted Jsonnet in 0m0.146s
Formatted Go in 0m0.065s
Formatted Shell in 0m0.004s
Formatted Protocol Buffer in 0m0.970s
Formatted C++ in 0m0.010s
Formatted YAML in 0m0.019s
Formatted Rust in 0m0.401s

As far as I can tell this only impacts rustfmt. We have most other formatters enabled and not seen similar problems.

The root cause is that the upstream_wrapper in rules_rust changes the working directory if the environment variable is set. See https://github.com/bazelbuild/rules_rust/blob/main/tools/upstream_wrapper/src/main.rs


Changes are visible to end-users: yes

  • Searched for relevant documentation and updated as needed: yes
  • Breaking change (forces users to change their own code or config): no
  • Suggested release notes appear below: no

Test plan

I have not been able to find automated test cases in rules_lint. I am happy to add to any test case, but I would need a pointer.

Manual testing:

  • A repo with rust files
  • Go into a subdirectory
  • Call bazel run //:format

We have been seening problems when the Rust formatter is called
from a subdirectory and not the workspace root directory.

The root cause is that rustfmt appears to look for the file list
based on the BUILD_WORKING_DIRECTORY instead of
BUILD_WORKSPACE_DIRECTORY or the current working directory of format.sh

This patch fixes the issue by unsetting the BUILD_WORKING_DIRECTORY
if format.sh has BUILD_WORKSAPCE_DIRECTORY is set.

This has been locally tested:

before:
Running in a subdirectory

```
> bazel run //:format
INFO: Invocation ID: c0ef6738-6805-4ee3-8d5a-ab5a7c393865
INFO: Analyzed target //:format (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //tools:format up-to-date:
  bazel-bin/tools/format.bash
INFO: Elapsed time: 0.351s, Critical Path: 0.03s
INFO: 2 processes: 2 internal.
INFO: Build completed successfully, 2 total actions
INFO: Running command line: bazel-bin/tools/format.bash
684 files left unchanged
Formatted Python in 0m0.016s
Formatted Starlark in 0m0.066s
Formatted Jsonnet in 0m0.144s
Formatted Go in 0m0.067s
Formatted Shell in 0m0.004s
Formatted Protocol Buffer in 0m0.978s
Formatted C++ in 0m0.010s
Formatted YAML in 0m0.019s
Error: file `a/lib.rs` does not exist
Formatted Rust in 0m0.020s
FAILED: A formatter tool exited with code 123
```

after:

```
> bazel run //:format
INFO: Invocation ID: 0b2f4cff-24af-474a-aa25-1a32d0ce74a9
INFO: Analyzed target //:format (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //tools:format up-to-date:
  bazel-bin/tools/format.bash
INFO: Elapsed time: 0.380s, Critical Path: 0.03s
INFO: 2 processes: 2 internal.
INFO: Build completed successfully, 2 total actions
INFO: Running command line: bazel-bin/tools/format.bash
684 files left unchanged
Formatted Python in 0m0.017s
Formatted Starlark in 0m0.066s
Formatted Jsonnet in 0m0.146s
Formatted Go in 0m0.065s
Formatted Shell in 0m0.004s
Formatted Protocol Buffer in 0m0.970s
Formatted C++ in 0m0.010s
Formatted YAML in 0m0.019s
Formatted Rust in 0m0.401s
```
@CLAassistant
Copy link

CLAassistant commented Jul 10, 2024

CLA assistant check
All committers have signed the CLA.

@alexeagle
Copy link
Member

Yeah I see https://github.com/bazelbuild/rules_rust/blob/main/tools/upstream_wrapper/src/main.rs#L32-L34

I'm not sure we should be invoking a tool from rules_rust - perhaps there's some vanilla distribution of rustfmt that we can run? I'm not very excited about making workarounds to undo customizations we didn't want to begin with.

@dmeister
Copy link
Contributor Author

@alexeagle thank you

@alexeagle alexeagle merged commit 67a433c into aspect-build:main Jul 15, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants