search

keith / buildifier-prebuilt

A bazel toolchain for using prebuilt binaries for buildifier and buildozer
MIT License
35 stars 13 forks source link

Not compatible with `--nolegacy_external_runfiles` #74

Closed MillerJhang closed 1 year ago

MillerJhang commented 1 year ago

Thanks for the work on this project! It's so great but I'm encountering an issue blocking me to continue with it.

Issue:

Reproduction:

  1. Have a simple Bazel WorkSpace, the following steps will take this one from rules_python as the example
  2. Change WORKSPACE lines 64-72 as instructions in line 63
  3. Integrate buildifier-prebuilt with the legacy-workspace-file method
  4. Create a rule for running buildifier
  5. Run "bazel run //:buildifier.check". it works.
  6. Choose one of the below two steps to operate: 6-1. Add build --nolegacy_external_runfiles into the file .bazelrc; I added it in the first line. Then run bazel run //:buildifier.check agan. or 6-2. Run "bazel run //:buildifier.check --nolegacy_external_runfiles"
  7. It does not work (no fixes found) now.
  8. (Optional) Add arguments --run_under="bash -x" and --sandbox_debug and run again for more details.

Expectation:

keith commented 1 year ago

Is the solution here to be using the shell snippet that you copy paste from bazel? should be fine to update if so

MillerJhang commented 1 year ago

No solution was provided from me 😅 But if you run the buildifier in the way mentioned as step 8 above, you can see that the line does readlink fails because the first dir is external whereas it maybe should be ../ because with the --nolegacy_external_runfiles the /external folder will not be created anymore.

++ readlink external/buildifier_darwin_arm64/file/buildifier

keith commented 1 year ago

can you add a repro case here? I can't repro this in our project. what version of bazel + buildifier prebuilt are you on

keith commented 1 year ago

or better yet a PR with a failing test

MillerJhang commented 1 year ago

The version of bazel and buildifier-prebuilt used in the reproduction steps in the start of this thread is 6.3.1 and 6.1.2

MillerJhang commented 1 year ago

I found that it can be fixed on my laptop after changing the first .path to .short_path in this line, and it works either specifying nolegacy_external_runfiles or not.

Should I create a PR of that, or any suggestion?

keith commented 1 year ago

worth a try with a PR but I think that was just changed to support windows

keith commented 1 year ago

https://github.com/keith/buildifier-prebuilt/pull/77