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

WORKSPACE load statements are reordered when reading from stdin #1268

Open
mkosiba opened this issue Apr 24, 2024 · 3 comments
Open

WORKSPACE load statements are reordered when reading from stdin #1268

mkosiba opened this issue Apr 24, 2024 · 3 comments

Comments

@mkosiba
Copy link

mkosiba commented Apr 24, 2024

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
Copy link
Member

vladmos commented May 3, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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

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

No branches or pull requests

4 participants