facebookincubator/buck2-change-detector

`targets` returns 0 when parsing errors occur

Closed this issue · 2 comments

cbarrete commented

targets skips invalid BUCK files instead of failing the process, causing errors introduced in those files not to be caught in CI.

We have caught this after buildifier mangled a BUCK file but the CI still passed.

A workaround for now is to just add buck2 targets //... in our CI so that the job will fail if some files are invalid, but this is a hack and I believe should at least conditionally support erroring out on invalid files.

Minimal repro:

BUCK file

cxx_binary()

supertd targets :

Error parsing root//
Error evaluating build file: `root//:BUCK`

Caused by:
    Traceback (most recent call last):
      * BUCK:1, in <module>
          cxx_binary()
    error: Missing parameter `name` for call to cxx_binary
     --> BUCK:1:1
      |
    1 | cxx_binary()
      | ^^^^^^^^^^^^
      |
    
Build ID: 140c13b8-53aa-4cfe-8517-4f7966e402d6
Jobs completed: 4. Time elapsed: 0.0s.
{"buck.package":"root//","buck.error":"Error evaluating build file: `root//:BUCK`\n\nCaused by:\n    Traceback (most recent call last):\n      * BUCK:1, in <module>\n          cxx_binary()\n    error: Missing parameter `name` for call to cxx_binary\n     --> BUCK:1:1\n      |\n    1 | cxx_binary()\n      | ^^^^^^^^^^^^\n      |\n    "}

echo $?

0

However, buck2 targets : fails as expected, with return code 3.

ndmitchell commented

This behaviour is by design. BTD is designed for the situation where the repo is huge and some subset of it is almost always broken. If you pass supertd btd the result of supertd targets : on the base, and supertd targets : on the diff, it will fail if anything has been newly broken.

If you want to use BTD without that, run supertd targets : --dry-run, grab the suitable arguments, and remove --keep-going to cause failure more immediately. supertd targets is just a helper to run buck2 targets with the right flags. It isn't essential.

cbarrete commented

I figured you might say that, but I hadn't realized just how minimal supertd targets was, I'll just make my own version then. Thank you!

  • 👍1