bazelbuild / buildtools

A bazel BUILD file formatter and editor
Apache License 2.0
1.03k stars 418 forks source link

WORKSPACE load statements are reordered when reading from stdin #1268

Open mkosiba opened 6 months ago

mkosiba commented 6 months ago

Steps to reproduce:

  1. Find a WORKSPACE. I think any file with multiple load statements will do, but this is the one I used to repro:

    load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
    
    git_repository(
        name = "rules_python",
        sha256 = "c68bdc4fbec25de5b5493b8819cfc877c4ea299c0dcb15c244c5a00208cde311",
        strip_prefix = "rules_python-0.31.0",
        url = "https://github.com/bazelbuild/rules_python/releases/download/0.31.0/rules_python-0.31.0.tar.gz",
    )
    
    load("@rules_python//python:repositories.bzl", "py_repositories")
    
    py_repositories()
  2. run buildozer in stdin mode: cat WORKSPACE | buildozer 'set name rules_python' '-:rules_python'
  3. Observe load statements are reordered. When running against the file directly this does not reproduce.

This is a regression from buildozer 7.0.0 onwards. buildozer 6.4.0 doesn't exhibit this behaviour.

vladmos commented 6 months ago

Buildozer now reorders load statements for all file types except for WORKSPACE files. The problem here is that buildozer doesn't know that the file is called WORKSPACE because it's been passed via stdin.

I think the correct way of fixing it is to disable moving load statements on top if the filename is not known (or if it's a random temp file name that's neither BUILD- nor WORKSPACE-like). A downside would be that if you pass a BUILD file with load statements placed randomly in the file, buildozer won't make it complying the formatting rules, but that should be 1) a rare case, and 2) smaller problem than making a file incorrect and breaking builds.

laurentlb commented 6 months ago

I think we should provide a way to do step 2 without using stdin. This will be more convenient for the users, and it will give much more information to Buildifier (e.g. know the path).

In general, load statements can be moved (according to the Starlark spec), workspace files are just a weird corner case.

illicitonion commented 6 months ago

buildifier has a --type flag which takes build|bzl|workspace|default|module|auto - maybe we could introduce the same to buildozer?

loeffel-io commented 1 month ago

for my very specific nvim conform case i fixed this by

                ["buildifier"] = {
                    stdin = false,
                    prepend_args = { "--lint", "fix", "$FILENAME" },
                },