Python rules ignore "imports" provider passed in from Starlark-defined rules

[brandjon]

From what I can tell from reading the code and some quick experimentation, it looks like we pull the imports from the native PythonImportsProvider and from BazelPythonSemantics#getImports (which reads the imports attribute), but not from the imports field of a py provider struct of a dependency. This is a simple fix, but we can probably combine it with a cleanup that removes PythonImportsProvider altogether. Won't address until after we deprecate the py legacy struct provider.

[ovy]

Hi guys,

I am in dire need of a fix for this right now. The fact that the imports attribute was being silently ignored, coupled with the transitionary state of the py provider docs, has already lead to much hair pulling in my proposed migration to Bazel. Is there any chance you can prioritize it, even if that means making it PyInfo-only ("modern-only") ** ? I really don't know how to make imports work properly otherwise. A py_library wrapper seems like a non-starter because of the other transitive information we need to propagate.

Best,
Ovy

Edit: to clarify, a world in which PyInfo.imports works but "py".imports doesn't still seems much better than a world in which neither works. So we don't have to wait until the disallow_legacy is default, I think. Or am I misunderstanding and PyInfo.imports will work in 0.23?

[brandjon]

When I was refactoring how PyCommon manages the Python provider(s), I made a conscious decision to not try to bugfix / alter semantics, for fear of introducing regressions that would require their own incompatible changes and complicate the migration to PyInfo. It's not actually any harder to fix it for both the legacy provider and PyInfo than for just one or the other.

Looking over the rule logic again, it seems that this is simpler than I remember. (Refactorings have that effect after all...) PythonImportsProvider -- which is the native code's way of passing imports around, and the one that the rules actually listen to -- is only ever created and used in a straightforward way. There's really no reason we can't replace it with the imports field of the legacy and PyInfo providers.

Given the ease of the fix vs the benefit, I'll prioritize this. Should appear in 0.24 (the release after the current RC).

[ovy]

Thanks Jon! I appreciate the quick response and offer to fix. I wish it was sooner than 0.24 though... any chance it's simple enough (as well as being pure forward progress) to be in .23?

I also look forward to the py provider matching the others.

Once this is done, I'll probably contribute a nice chunk of Starlark (for interop with ROS) back to the community, so this is good karma all around.

[brandjon]

Unfortunately the baseline for 0.23 has already been cut, and we tend not to cherry pick feature requests in, as opposed to bug fixes. In the meantime you can try it out with bazel built at head. Watch this bug to see when the fix is in.

[brandjon]

Also, you should still be able to add imports using the regular old py_library rule. Maybe you can combine that with your rule in a macro to hack around this bug in the meantime.

[brandjon]

Fix is in if you wanted to grab a dev build.