search

pantsbuild / pants

The Pants Build System
https://www.pantsbuild.org
Apache License 2.0
3.34k stars 640 forks source link

`--changed` may not detect an inferred dependency on a deleted (or malformed) file #14975

Open stuhood opened 2 years ago

stuhood commented 2 years ago

~Although we have yet to see it in production,~ (EDIT: See #17512) currently dependency inference may prevent --changed from finding any dependents of a missing or malformed file.

With explicitly specified dependencies in BUILD files, removing a file and not also updating the dependent BUILD files will cause BUILD files to fail to parse. But with dependency inference, unless [python-infer].unowned_dependency_behavior=error is set, inference @rules will silently not find the source of a symbol.

To resolve this, we should probably attempt to bias toward defaulting to warning (and possibly erroring) for unrecognized imports, across all languages.

stuhood commented 2 years ago

@thejcannon : So it turns out that unowned_dependency_behavior might actually be something that would be worth enabling by default. How was your experience with enabling it in your repo, and how painful do you think that it would be to enable it at warn?

thejcannon commented 2 years ago

I turned it on and never looked back! I think defaulting to warn makes sense.

There's a discussion to have about where it should happen. E.g in it's own "lint"er or "check"er, but that's extra

stuhood commented 2 years ago

There's a discussion to have about where it should happen. E.g in it's own "lint"er or "check"er, but that's extra

Given the OP, I would be concerned that if it were in lint or check you might silently not run lint or check on the relevant dependees when using --changed.

jriddy commented 1 year ago

I have an edge case example of where [python-infer].unowned_dependency_behavior = "error" is not enough to solve this problem: conftest.

Assuming [python-infer].conftest = true, these conftest.py are magically inferred dependencies with no corresponding import statement in the dependent modules, meaning there's no unowned dependency declaration to error on when that conftest is removed.

I'd argue that the implicit dependency on conftest by pytest is a malignant anti-pattern, but it's a well-established one, so it's hard to just say "don't do it that way." But I don't see a way to fix this, as there's no information for Pants to know that a module used to implicitly depend on a module that's no longer there.