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

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

Open
stuhood opened this issue Mar 31, 2022 · 4 comments
Labels

Comments

@stuhood
Copy link
Member

stuhood commented Mar 31, 2022

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 stuhood added the bug label Mar 31, 2022
@stuhood
Copy link
Member Author

stuhood commented Mar 31, 2022

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

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

stuhood commented Mar 31, 2022

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.

stuhood added a commit that referenced this issue Jul 26, 2022
…ult. (#16281)

Rough consensus was reached on #15326 and #14975 that enabling `unowned_dependency_behavior="warning"` by default would:
1. be helpful for new users, to guide them through fixing their missing dependencies
2. be useful in the case of adding additional resolves (due to the work done in #15326 to enrich the warning for that case)
3. reduce the chances of CI errors in #14975 (when users go a step further to make the warning an error)

`2.14.x` is a relatively quiet release so far, so it seems like there is room in the budget for a new warning like this.

The rendered message for this warning is very self-explanatory due to @thejcannon's original work, and @Eric-Arellano's followup work in #15326, so I don't see any obvious documentation changes that need to be made.

[ci skip-build-wheels]
[ci skip-rust]
@jriddy
Copy link
Contributor

jriddy commented Dec 9, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants