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.

Sign up for GitHub

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

Jump to bottom

Switch Python rules to use a proper starlark-accessible provider #7010

Closed
6 tasks done
brandjon opened this issue Dec 28, 2018 · 6 comments
Closed
6 tasks done

Switch Python rules to use a proper starlark-accessible provider #7010

brandjon opened this issue Dec 28, 2018 · 6 comments
Assignees
@brandjon
Labels
P1 I'll work on this now. (Assignee required) team-Rules-Python Native rules for Python type: process

Comments

@brandjon
Copy link
Member

brandjon commented Dec 28, 2018 •
edited
Loading

The py legacy-style string-named provider should be replaced by a PyInfo provider type, whose fields are enumerated and typed. This should also subsume the native PythonImportsProvider type.

This should more or less implement "Python sandwich" -- having Starlark-defined-rules depend on the native Python rules and vice versa.

  • Add helper to abstract over which provider type is used and default value logic
  • Implement the new provider type, make Python rules consume and return it
  • Add incompatible flag for making Python rules disallow and not return the old provider
  • Make migration-ready
  • Migrate Bazel's own tests
  • Migrate downstream and flip the flag
@brandjon brandjon added P2 We'll consider working on this in future. (Assignee optional) type: process team-Rules-Python Native rules for Python labels Dec 28, 2018
@brandjon
Copy link
Member Author

brandjon commented Jan 3, 2019

Note that some form of Starlark sandwich is already possible. For instance, I can write a py_library replacement that exposes its srcs in a transitive_sources field. We need to document an exact API.

To accelerate/unblock work on migrating away from the legacy struct providers, we might decouple the change in the provider type from the change in the provider's fields. I.e., we could have one incompatible change flag to deprecate ctx.attr.my_dep.py with ctx.attr.my_dep[PyInfo], and another incompatible change flag that affects the names or types of a bunch of fields on PyInfo.

bazel-io pushed a commit that referenced this issue Jan 4, 2019
@brandjon
Refactor "py" provider implementation as an ADT
455caab 
The py provider is currently just a struct manipulated by native code. Eventually it should be a full-fledged Provider, but in the meantime this moves its accessors to a separate utility class.

This is preparation for adding new provider fields to address #6583 and #1446. The cleanup is also a small step toward #7010.

RELNOTES: None
PiperOrigin-RevId: 227771462
@brandjon
Copy link
Member Author

Related issues: #7054 and #7121.

bazel-io pushed a commit that referenced this issue Jan 15, 2019
@brandjon
Refactor PyProvider helper utilities
3c26f9e 
PyProvider manages how Java code creates and accesses the fields of the legacy "py" struct provider (which we'll migrate away from soonish). This CL refactors default value logic into PyProvider, and adds a builder so it's easier to construct. It also slightly refactors some shared-library logic in PyCommon. More cleanup (hopefully) to come.

Work toward #7010, yak shaving for #6583 and #1446.

RELNOTES: None
PiperOrigin-RevId: 229401739
bazel-io pushed a commit that referenced this issue Jan 15, 2019
@brandjon
Refactor how PyCommon manages its transitive sources
9ba4663 
Simplify computing the transitive sources and transitive-without-local sources by factoring a method or two and separating out the computation of `convertedFiles`.

Work toward #7010, yak shaving for #6583 and #1446.

RELNOTES: None
PiperOrigin-RevId: 229410930
bazel-io pushed a commit that referenced this issue Jan 15, 2019
@brandjon
Regroup PyCommon methods for consistency
59038cd 
PyCommon's initialization and validation helpers are renamed and moved to near the top of the class, followed by simple accessors. Initialization methods are made static so there are no longer any implicit dependencies on which fields get initialized first.

Python import paths are now computed in the PyCommon constructor in order to follow the pattern of other provider fields. This requires passing the PythonSemantics to the constructor, so while I was at it I made PyCommon keep a reference to the semantics. I was a little concerned about this promoting increased coupling between PyCommon and PythonSemantics, but given that both classes serve essentially the same purpose I'm not too worried.

Work toward #7010, yak shaving for #6583 and #1446.

RELNOTES: None
PiperOrigin-RevId: 229425980
@brandjon
Copy link
Member Author

Will work on this next. For the purposes of this tracking bug, the contents of the provider are out-of-scope; we only care about accessing it as a modern provider instead of a legacy one.

@brandjon brandjon added P1 I'll work on this now. (Assignee required) and removed P2 We'll consider working on this in future. (Assignee optional) labels Jan 22, 2019
@brandjon brandjon self-assigned this Jan 24, 2019
bazel-io pushed a commit that referenced this issue Jan 24, 2019
@brandjon
Rename the PyProvider helper class to PyStructUtils
497407c 
In follow-up CLs, we'll introduce a new modern-style provider named PyInfo, and a new helper class PyProviderUtils to give a unified view for accessing either provider. The old name PyProvider would have been confusing.

Work toward #7010.

RELNOTES: None
PiperOrigin-RevId: 230759883
bazel-io pushed a commit that referenced this issue Jan 24, 2019
@brandjon
Move depset order validation from PyCommon to PyStructUtils
14c1793 
The fact that the field is a depset is already an invariant of the provider, so we may as well include the order in that requirement. This is not a breaking change because it's currently enforced by PyCommon.

Moving the validation out of PyCommon will make it easier to refactor the "get this field from that dep" helpers out of PyCommon and into a shared PyProviderUtils helper.

Work toward #7010.

RELNOTES: None
PiperOrigin-RevId: 230780935
bazel-io pushed a commit that referenced this issue Jan 24, 2019
@brandjon
Add helper for abstracting over Python provider representation and de…
4b3725e 
…faults

This creates PyProviderUtils, a collection of helpers that take in a TransitiveInfoCollection and return fields obtained from the Python provider. Currently this just dispatches to PyStructUtils to retrieve from the legacy "py" struct provider. In a follow-up CL, we'll add a modern PyInfo type, and PyProviderUtils will access either one as appropriate.

PyProviderUtils also takes over the default logic from PyCommon, for when no Python provider information is available.

Work toward #7010.

RELNOTES: None
PiperOrigin-RevId: 230791581
bazel-io pushed a commit that referenced this issue Jan 29, 2019
@brandjon
Introduce PyInfo to replace the legacy "py" struct provider
ddfc430 
This CL adds the PyInfo data type and registers it with a bootstrap. A follow-up CL will make the Python rules actually produce and consume this provider.

The new provider's API exactly mirrors the current API of the legacy struct provider, as encapsulated by PyStructUtils.

Work toward #7010.

RELNOTES: None
PiperOrigin-RevId: 231460993
@brandjon
Copy link
Member Author

Change of plan: The incompatible flag will actually disallow passing the legacy provider to native Python rules, not just make it ignored. Failing fast makes it easier to find rules that need migrating, and avoids subtle bugs due to falling back on default logic.

bazel-io pushed a commit that referenced this issue Jan 29, 2019
@brandjon
Make Python rules use new PyInfo provider
a092b69 
This makes PyProviderUtils and the Python rules aware of the new PyInfo provider, which will replace the legacy "py" struct provider. With this CL, both providers are produced by the rules, and either can be consumed from a dependency. (If both providers are present on a dependency then the legacy provider is ignored.)

PyProviderUtils gets a builder that dispatches to the legacy and modern builders. In a follow-up CL we'll use this new builder to gate whether or not the legacy provider gets produced.

Work toward #7010.

RELNOTES: None
PiperOrigin-RevId: 231471616
@brandjon brandjon mentioned this issue Jan 30, 2019
Closed
weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
@brandjon @weixiao-huang
Refactor PyProvider helper utilities
72472b6 
PyProvider manages how Java code creates and accesses the fields of the legacy "py" struct provider (which we'll migrate away from soonish). This CL refactors default value logic into PyProvider, and adds a builder so it's easier to construct. It also slightly refactors some shared-library logic in PyCommon. More cleanup (hopefully) to come.

Work toward bazelbuild#7010, yak shaving for bazelbuild#6583 and bazelbuild#1446.

RELNOTES: None
PiperOrigin-RevId: 229401739
weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
@brandjon @weixiao-huang
Refactor how PyCommon manages its transitive sources
d1f482d 
Simplify computing the transitive sources and transitive-without-local sources by factoring a method or two and separating out the computation of `convertedFiles`.

Work toward bazelbuild#7010, yak shaving for bazelbuild#6583 and bazelbuild#1446.

RELNOTES: None
PiperOrigin-RevId: 229410930
weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
@brandjon @weixiao-huang
Regroup PyCommon methods for consistency
d8c441f 
PyCommon's initialization and validation helpers are renamed and moved to near the top of the class, followed by simple accessors. Initialization methods are made static so there are no longer any implicit dependencies on which fields get initialized first.

Python import paths are now computed in the PyCommon constructor in order to follow the pattern of other provider fields. This requires passing the PythonSemantics to the constructor, so while I was at it I made PyCommon keep a reference to the semantics. I was a little concerned about this promoting increased coupling between PyCommon and PythonSemantics, but given that both classes serve essentially the same purpose I'm not too worried.

Work toward bazelbuild#7010, yak shaving for bazelbuild#6583 and bazelbuild#1446.

RELNOTES: None
PiperOrigin-RevId: 229425980
weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
@brandjon @weixiao-huang
Rename the PyProvider helper class to PyStructUtils
d75e7ea 
In follow-up CLs, we'll introduce a new modern-style provider named PyInfo, and a new helper class PyProviderUtils to give a unified view for accessing either provider. The old name PyProvider would have been confusing.

Work toward bazelbuild#7010.

RELNOTES: None
PiperOrigin-RevId: 230759883
weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
@brandjon @weixiao-huang
Move depset order validation from PyCommon to PyStructUtils
c119d2a 
The fact that the field is a depset is already an invariant of the provider, so we may as well include the order in that requirement. This is not a breaking change because it's currently enforced by PyCommon.

Moving the validation out of PyCommon will make it easier to refactor the "get this field from that dep" helpers out of PyCommon and into a shared PyProviderUtils helper.

Work toward bazelbuild#7010.

RELNOTES: None
PiperOrigin-RevId: 230780935
weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
@brandjon @weixiao-huang
Add helper for abstracting over Python provider representation and de…
f4492e4 
…faults

This creates PyProviderUtils, a collection of helpers that take in a TransitiveInfoCollection and return fields obtained from the Python provider. Currently this just dispatches to PyStructUtils to retrieve from the legacy "py" struct provider. In a follow-up CL, we'll add a modern PyInfo type, and PyProviderUtils will access either one as appropriate.

PyProviderUtils also takes over the default logic from PyCommon, for when no Python provider information is available.

Work toward bazelbuild#7010.

RELNOTES: None
PiperOrigin-RevId: 230791581
weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
@brandjon @weixiao-huang
Introduce PyInfo to replace the legacy "py" struct provider
3c67250 
This CL adds the PyInfo data type and registers it with a bootstrap. A follow-up CL will make the Python rules actually produce and consume this provider.

The new provider's API exactly mirrors the current API of the legacy struct provider, as encapsulated by PyStructUtils.

Work toward bazelbuild#7010.

RELNOTES: None
PiperOrigin-RevId: 231460993
weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
@brandjon @weixiao-huang
Make Python rules use new PyInfo provider
33393e5 
This makes PyProviderUtils and the Python rules aware of the new PyInfo provider, which will replace the legacy "py" struct provider. With this CL, both providers are produced by the rules, and either can be consumed from a dependency. (If both providers are present on a dependency then the legacy provider is ignored.)

PyProviderUtils gets a builder that dispatches to the legacy and modern builders. In a follow-up CL we'll use this new builder to gate whether or not the legacy provider gets produced.

Work toward bazelbuild#7010.

RELNOTES: None
PiperOrigin-RevId: 231471616
bazel-io pushed a commit that referenced this issue Jan 31, 2019
@brandjon
Add flag to support PyInfo provider migration
8e3b3b1 
This adds --experimental_disallow_legacy_py_provider, which 1) makes native Python rules no longer emit a "py" struct provider, and 2) makes these rules fail-fast if any direct dependency returns a "py" struct provider. Rules should upgrade to PyInfo instead.

Once we add suitable documentation / tracking bugs, we'll rename to --incompatible_[...].

Work toward #7010.

RELNOTES: None
PiperOrigin-RevId: 231839371
bazel-io pushed a commit that referenced this issue Feb 1, 2019
@brandjon
Add PyInfo migration flag to --all_incompatible_changes
29759d0 
This renames --experimental_disallow_legacy_py_provider to --incompatible_disallow_legacy_py_provider and makes it available under --all_incompatible_changes. Migrate legacy "py" struct providers to PyInfo instead.

See #7298 for more information.

Work toward #7298 and #7010.

RELNOTES[INC]: Python rules will soon reject the legacy "py" struct provider (preview by enabling --incompatible_disallow_legacy_py_provider). Upgrade rules to use PyInfo instead. See [#7298](#7298).

PiperOrigin-RevId: 231885505
@brandjon
Copy link
Member Author

brandjon commented Feb 2, 2019

At this point the feature is fully implemented (to be in 0.23), and this issue is now blocked on migration and flipping the incompatible change flag (#7298), then cleanup of the legacy code paths.

@brandjon
Copy link
Member Author

Cleanup of legacy code tracked by #7741.

bazel-io pushed a commit that referenced this issue Apr 6, 2020
@brandjon
Rewrite TODOs to use internal bug id
97f9a93 
Issue #7010 has long been closed in OSS Bazel, but its Google-internal equivalent remains open, pending a cleanup of the depot.

RELNOTES: None
PiperOrigin-RevId: 305103124
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@brandjon brandjon

Labels
P1 I'll work on this now. (Assignee required) team-Rules-Python Native rules for Python type: process
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@brandjon