Rely on the init-injection rules to trigger errors for un-depended-on and non-empty __init__.py files

[stuhood]

Problem

#10517 restored the automatic inclusion of empty __init__.py files, and additionally adds an error for "un-depended-on and non-empty __init__.py files". But --python-infer-inits also triggered an error for an unowned __init__.py file, regardless of whether it had content, which meant that if you had inference on, you would see errors from inference before seeing the error that differentiates between empty and non-empty.

Solution

Remove the error for un-owned __init__.py files from --python-infer-inits and rely on the error from __init__.py injection. Additionally, expand the error for __init__.py injection to clarify that you need an owning target.

Result

--python-infer-inits won't complain about empty __init__.py files not being owned.

[ci skip-rust]
[ci skip-build-wheels]

[benjyw]

So this interacts with setup-py in odd ways, which I've been banging my head against all morning. I need to go clear my head and eat lunch, so will look at this in a bit. I do think that removing the check is the right thing to do. Will write more once I get my thinking cap on.

[Eric-Arellano]

Thanks! Going to wait for Benjy to review. He realized that we probably shouldn't be erroring on files with content in the first place.

[benjyw]

First thought is this: "empty" should also have included "just a namespace package invocation". In both cases it's fine to just grab them without a dep.

In the setup-py case, it's actually even necessary to do so: With setup-py, the dist can only export code from libraries that are below it in the source tree, according to the rules in https://www.pantsbuild.org/v2.0/docs/python-setup-py-goal#mapping-libraries-to-distributions. So we have to special-case __init__.py files that are above it, we can't just treat them like regular deps. And note that those ancestor __init__.py files will be pulled in to multiple dists, so they need to be trivial (empty or ns package) - if they contain code, that code will be in multiple dists, which means its deps need to be, which is problematic.

So, if you have such a situation, you are responsible for your __init__.py hygiene, which leads me to believe that we shouldn't check them at all. We pull the ancestors in, either via deps or via injection, and the author is responsible for the content working with that scheme.

[stuhood]

First thought is this: "empty" should also have included "just a namespace package invocation". In both cases it's fine to just grab them without a dep.

Possibly: I think that I don't know enough about them to be sure. But I think that this change might make sense regardless, because it gets out of the way and lets the "automatic injector" rules make that decision and decide whether to error.

[Eric-Arellano]

I agree that this is better behavior and that it makes sense to land regardless of Benjy's changes. Thanks!

[benjyw]

Yes, I think this is a good change, and interacts well with my upcoming change.

To clarify my understanding, if --python-infer-inits=True and --python-infer-imports=False then we'll infer deps on ancestor init files, but not through them to their deps? So in that case if you have __init__.py files with deps you have to have an explicit target and explicit deps for it, but you don't have to depend on that target explicitly?

[Eric-Arellano]

To clarify my understanding, if --python-infer-inits=True and --python-infer-imports=False then we'll infer deps on ancestor init files, but not through them to their deps? So in that case if you have init.py files with deps you have to have an explicit target and explicit deps for it, but you don't have to depend on that target explicitly?

This change means that—regardless of if there's content in the files or not—we will not infer __init__.py if there are no owners. The dep inference rule will ignore those files. Then, your new logic will kick in and make sure they get included.

Currently, your inject init will error if there is already content. Soon, if you stick to your proposal, we won't complain about those inits even if they have content.

[stuhood]

To clarify my understanding, if --python-infer-inits=True and --python-infer-imports=False then we'll infer deps on ancestor init files, but not through them to their deps?

We will pick up the deps of any literal target that owns the __init__.py file (which might be a significant number if you're not using --infer-imports). So yea, that aspect remains non-ideal. But the primary benefit of this change is avoiding needing to turn off --python-infer-inits due to errors... you might still turn it off if you want fewer dependencies.

[benjyw]

This is the change I was referring to: #10529