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

Port Python rules to Starlark #15897

Open
14 of 15 tasks
rickeylev opened this issue Jul 17, 2022 · 8 comments
Open
14 of 15 tasks

Port Python rules to Starlark #15897

rickeylev opened this issue Jul 17, 2022 · 8 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Rules-Python Native rules for Python type: bug

Comments

@rickeylev
Copy link
Contributor

rickeylev commented Jul 17, 2022

This is the public tracking issue for the effort to port the Python rules from Java to Starlark.

I'll provide status updates here upon request or when there are particularly notable developments.

Internally, there's been a lot of progress on this. The work has stayed internal so far simply because, internally, Google has quite a few modifications to the rules and I am not/wasn't entirely sure how feasible things would be in practice, and I simply don't have time to, or experience in, managing the Google->Github side of things. Things have gone pretty good overall, though; we're very close to enabling py_binary and py_test within Google.

The rough plan is to re-implement the rules using the "builtins" mechanism of Bazel. This basically allows Bazel to bundle Starlark code with itself and provides mechanisms for Java<->Starlark interop, in particular ways for Starlark to call private APIs the Bazel team isn't sure should be generally public or not. While this means you would still have to wait for Bazel release to get changes, modifications should be much easier and accessible.

When and how to transition out of Starlark builtins to e.g. rules_python, is still TBD. That's probably going to depend on behavioral differences and the extent of reliance on the Java<->Starlark builtins interop.

Status (March 2023)

  • All the rules are using the Starlark implementation
  • Porting providers is in-progress

Within Google, we have the Starlark implementation enabled. So rewriting in Starlark is possible and works; just a matter of tracking down edge cases now.

Blockers

  • None at the moment

Incompatibilities (or other observable changes in behavior)

  • No python 2 support: as part of Bazel 7, support for Python 2 is being removed.
    This applies to the Java-implemented rules, too.
  • --experimental_build_transitive_python_runfiles=true not supported: This is
    being dropped as part of Bazel 7 and applies to the Java-implemented rules, too.
    This feature enables an obscure behavior where building one target materializes
    the runfiles of dependent binaries as if they were directly built.
  • stamp attribute can no longer be a configurable (select-based) boolean value.
    Integers (select-based or not) are still supported (and preferred).
  • legacy_create_init attribute can no longer be a configurable (select-based boolean
    value. Integers (select-based or not) are still supported (and preferred).
  • In cquery commands, the provider name as returned by the providers() function
    has changed. These "new" names are considered internal and should not be relied upon;
    they will change again when the rules are removed from Bazel and moved to the
    rule_python repository.
    • PyInfo -> @_builtins//:common/python/providers.bzl%PyInfo
    • PyRuntimeInfo -> @_builtins//:common/python/providers.bzl%PyRuntimeInfo
  • For py_test to add the requires-darwin execution requirement, the Apple
    OS constraints (@platforms//os:{ios,macos,tvos,watchos}) must match. Setting
    --cpu flag (e.g --cpu=darwin_x86_64) isn't sufficient. Note that this only
    affects remotely running a py_test built for Apple platforms.

TODO

  • Implement imports attribute
  • PyInfo provider
  • PyRuntimeInfo provider
  • PyCcLinkParamsProvider provider
  • Port flags to Starlark
  • py_binary / py_test
  • py_library
  • py_runtime
  • Re-enable allowing using the Starlark impl for tests where it was disabled for Google-internal reasons (e.g. imports attribute)
  • Bazel regression tests passing
  • Python 2 disabled
  • experimental_build_transitive_python_runfiles disabled
  • Enable the rules
  • Enable the providers
  • Fix downstream project failures

cc: @comius @brandjon

@UebelAndre
Copy link
Contributor

Thanks for filing this! Is there also a tracking issue for the cc rules port (and I suppose all the other built in rules)?

@comius comius added P2 We'll consider working on this in future. (Assignee optional) and removed untriaged labels Jul 19, 2022
@rickeylev
Copy link
Contributor Author

rickeylev commented Oct 4, 2022

Per request, I've put together a public doc that talks about hurdles for moving the implementation over to Starlark. There's still more in the internal doc I need to move over, but it currently covers the major parts.

https://docs.google.com/document/d/1-dC4QAxX67r7qK77D5nrnpykBgZEN-I9KBGTBVZba4M/edit?resourcekey=0-jYhsNDu8EKga8kTuMX4u9Q#

edit: corporate security policies prevent me from making the doc actually public. Press the "request access" button and I will grant access.

copybara-service bot pushed a commit that referenced this issue Oct 13, 2022
This is to allow the Starlark implementation to register the extra actions.

Work towards #15897

PiperOrigin-RevId: 480954594
Change-Id: Idd10e1f135211ec163a1a12d65626f4a7f0f7a78
copybara-service bot pushed a commit that referenced this issue Oct 17, 2022
Work towards #15897

PiperOrigin-RevId: 481569823
Change-Id: Iea3e55fce0bdb31e3b07a5e1bf0242cb6e0ba89b
copybara-service bot pushed a commit that referenced this issue Oct 17, 2022
This makes the test pass with the Starlark implementation enabled. This test
doesn't seem to really care what PyInfo state is, just that the provider is
found, so just check for that.

Work towards #15897

PiperOrigin-RevId: 481713979
Change-Id: I4c8e8d2b721a2ca2cc477e5e4151607534647732
copybara-service bot pushed a commit that referenced this issue Oct 19, 2022
The skip is based on product name because Bazel isn't yet using the Starlark
implementation.

Work towards #15897

PiperOrigin-RevId: 482060695
Change-Id: Idfbd615b55f805ecc2a9f4ba83e23df21ce94189
copybara-service bot pushed a commit that referenced this issue Oct 19, 2022
 * Most of this is due to Python 2 support (which is being dropped as part of
   the Starlark rewrite).
 * Also disables the Starlark impl for tests of the `imports` attribute
   (this is disabled within Google, and not yet implemented).

Work towards #15897

PiperOrigin-RevId: 482060859
Change-Id: I0c839c8cb20624d804d8d87e10170011ac6a3e55
@GregBowyer
Copy link

@rickeylev does this cover hermetic toolchains ala bazelbuild/rules_python#691 ?

@rickeylev
Copy link
Contributor Author

The toolchain rule and providers will also move into Starlark, yes.

@GregBowyer
Copy link

Sorry I will be more clear, with the rule and providers does this go towards working through your concerns and considerations as per bazelbuild/rules_python#691 (comment)

I ask because right now, we run a very hermetic world using bazel and NixOS, but this leads us to hacks like ticketmaster/rules_python@299cb89.

In a twist of fate we would be totally hermetic and deterministic with NixOS, but bazel actions tend to (rightfully) eat the $PATH which makes things unable to perform /usr/bin/env python3

These hacks work; however, there are limitations with caches, remoting and deterministic builds that would be good to start resolving.

@rickeylev
Copy link
Contributor Author

Ah ok. The rewrite into Starlark doesn't directly address those problems. It does make it much easier to address those problems, though, because (1) you can use Starlark instead of Java, and (2) changes won't be coupled to Bazel releases

copybara-service bot pushed a commit that referenced this issue Oct 22, 2022
Subsequent changes will begin adding actual functionality. This just creates
stubs than lets the `--experimental_builtins_bzl_path` and
`--experimental_builtins_injection_override` flags allow working with the
builtins code.

Work towards #15897

PiperOrigin-RevId: 483066291
Change-Id: If297913ddbef3e32aada41e7d319c87213921f9d
copybara-service bot pushed a commit that referenced this issue Oct 25, 2022
The implementation is still a no-op.

Work towards #15897

PiperOrigin-RevId: 483711468
Change-Id: Ia13d7ceeba7a3a2bc1e416fe94f46ac5f7ee0f35
copybara-service bot pushed a commit that referenced this issue Oct 25, 2022
Work towards #15897

PiperOrigin-RevId: 483722548
Change-Id: Ieee7859e516a946286eeb07f9a86d1ea2b159aa0
copybara-service bot pushed a commit that referenced this issue Oct 31, 2022
Various fixes from the rushed initial implementation
 * ctx.attr.files is a list of targets, not a single target
 * interpreter_path defaults to an empty string when not specified, while
   it must be None for PyRuntimeInfo to accept the non-hermetic case.
 * The files arg must be None for the non-hermetic case (an empty depset is
   not accepted by PyRuntimeInfo).
 * Missing the `.py` part when access the fragment to get the default
   Python version.
 * Default `python_version` to `_INTERNAL_SENTINEL` to match existing
   behavior (otherwise it defaults to empty string)

Work towards #15897

PiperOrigin-RevId: 485109694
Change-Id: Icfbbf606acb8074c12e7dba9b3ec39e6ac524bc5
copybara-service bot pushed a commit that referenced this issue Oct 31, 2022
…tarlark to use Java helpers

* `py_builtins` is an Starlark builtins-internal-only namespace for functions
  that only builtins should use.
* `py_internal` is for regular Starlark for testing and other esoteric
   purposes. Usage of this is restricted to packages relating to the rule
   implementations to prevent using these internal APIs.

(These already exist in the Google-internal build; this makes them defined
in Bazel, too, though they're currently no-ops; followup CLs will begin moving
common code out of the Google-py_internal and into the common one).

Work towards #15897

PiperOrigin-RevId: 485146419
Change-Id: I71902e60471a4227c1993778bda60cbbe6e316f0
copybara-service bot pushed a commit that referenced this issue Oct 31, 2022
…rage tool.

Work towards #15897

PiperOrigin-RevId: 485161913
Change-Id: I65128d9371a3e2124b2a82143530d88ba23e49b7
copybara-service bot pushed a commit that referenced this issue Oct 31, 2022
This will be necessary to implement `py_runtime` in regular Starlark, so
expose it now so it's available later.

Work towards #15897

PiperOrigin-RevId: 485180942
Change-Id: I9b79b9d3938de80307a94b6db37569c0a577f8d6
copybara-service bot pushed a commit that referenced this issue Nov 3, 2022
I manually built a Bazel and ran tests in rules_python with it; looks to be OK.

Work towards #15897

PiperOrigin-RevId: 485978566
Change-Id: I79c209b095492c39b1fff97091425fc7af486b12
copybara-service bot pushed a commit that referenced this issue Nov 3, 2022
This is a basic transcription of the PyRuntimeInfo class; it isn't enabled, used,
or tested yet. Subsequent changes will address test failures.

Work towards #15897

PiperOrigin-RevId: 485988691
Change-Id: Ic42b03bc6b82d04742273b6a7f331665dd91aee5
copybara-service bot pushed a commit that referenced this issue Nov 14, 2022
…Bazel

These will be necessary for the Starlarkified rules in Bazel.

Work towards #15897

PiperOrigin-RevId: 488420704
Change-Id: I10b0de454783ad976a14e3254d34bd13a2925876
copybara-service bot pushed a commit that referenced this issue Nov 15, 2022
Work towards #15897

PiperOrigin-RevId: 488684553
Change-Id: I6b439cf5b8fd75170e6647e4c76fa4ac8f72bdec
hvadehra pushed a commit that referenced this issue Feb 14, 2023
* Normalize the interpreter path. When the path is coming from another repo,
  it starts with "../", which the launcher resolution code doesn't know
  how to handle. Normalizing converts e.g. "workspace/../foo/bar" to "foo/bar".
* Implement the magic "py2 interpreter path sentinel overrides toolchain"
  logic. This logic is triggered on Windows to get "python" as the interpreter
  path that is put into the native launcher.

Work towards #15897

PiperOrigin-RevId: 505265140
Change-Id: I7beb6109739e23b884b818537c6948450458e2f7
hvadehra pushed a commit that referenced this issue Feb 14, 2023
There's a test that purposefully omits the attribute to test error messages.
To fix, just guard the access with hasattr().

Work towards #15897

PiperOrigin-RevId: 505715279
Change-Id: I4c8fe51783ac4f07e9c8ce32f457a65d941bf6b0
hvadehra pushed a commit that referenced this issue Feb 14, 2023
Not yet tested or exercised; to be done in a followup change.

Work towards #15897

PiperOrigin-RevId: 507902946
Change-Id: I0b203892d28e6b00f4ee6567e26abda84e6f71bc
hvadehra pushed a commit that referenced this issue Feb 14, 2023
Work towards #15897

PiperOrigin-RevId: 508420073
Change-Id: I0c681a0b70f0ea5111e3f8cca7b0c9cbe61e723a
hvadehra pushed a commit that referenced this issue Feb 14, 2023
This enables the Starlark implementation of the Python rules. While these are
largely drop-in replacements, there are a handful of edge cases that
are known to be incompatible; see #15897
for the list of cases.

If you believe the Starlark implementation to be the cause of a problem, please
post to #15897.

Work towards #15897

PiperOrigin-RevId: 509295132
Change-Id: Icbe5beb32e0700a915ac40fe4dbcacf102382974
copybara-service bot pushed a commit that referenced this issue Mar 6, 2023
This is to make transitioning the providers to Starlark easier.

Work towards #15897

PiperOrigin-RevId: 514463791
Change-Id: Iccd11f4750d3cf5ca5ed1b0ff8427d6580adcecc
copybara-service bot pushed a commit that referenced this issue Mar 7, 2023
With the deletion of the Java rule implementation, large amounts of code are
now unused. Entire removal of the classes isn't yet possible, but most of
their code can be removed.

This also makes it easier to transition to the Starlark providers, since less
Java code is referencing the to-be-deleted Java providers

Work towards #15897

PiperOrigin-RevId: 514566571
Change-Id: I28ef124369fc678945873bc21919830cb73eafc8
copybara-service bot pushed a commit that referenced this issue Mar 7, 2023
The Java rule implementation of them has been deleted.

Work towards #15897

PiperOrigin-RevId: 514769735
Change-Id: Ie7fa8e0f9d51943547319c6514edf33f8c47299c
copybara-service bot pushed a commit that referenced this issue Apr 10, 2023
Because providers are based on identity, and there are Java tests that need
to reference the provider, it's hard to split up the changes, so it's all
in a single change.

The Java class is kept to make usage by the Java tests easier; it isn't
actually used outside of tests.

Work towards #15897

PiperOrigin-RevId: 523182610
Change-Id: I6e07cf55ba08bb87c5ccb6cd37b473ed6aeeb760
copybara-service bot pushed a commit that referenced this issue Apr 12, 2023
Because providers are based on identity, and there are Java tests that need
to reference the provider, it's hard to split up the changes, so it's all
in a single change.

The Java class is kept to make usage by the Java tests easier; it isn't
actually used outside of tests.

Work towards #15897

PiperOrigin-RevId: 523748496
Change-Id: I1f5e9316bb3c4aed6c36fbd317914e4deea26f61
copybara-service bot pushed a commit that referenced this issue Apr 13, 2023
Because providers are based on identity, and there are Java tests that need
to reference the provider, it's hard to split up the changes, so it's all
in a single change.

A side-effect of this is the name used to access the provider in `cquery` commands
changes slightly: from "PyInfo" to "@_builtins//:common/python/providers.bzl%PyInfo". This fully qualified name is considered internal and shouldn't be relied upon; it will change again when
the rules are removed from Bazel and part of rules_python.

The Java class is kept to make usage by the Java tests easier; it isn't
actually used outside of tests.

Work towards #15897

PiperOrigin-RevId: 524054712
Change-Id: I1ab7d299c7b0458e0ee5359babb1fa63f9739498
fweikert pushed a commit to fweikert/bazel that referenced this issue May 25, 2023
This is to make transitioning the providers to Starlark easier.

Work towards bazelbuild#15897

PiperOrigin-RevId: 514463791
Change-Id: Iccd11f4750d3cf5ca5ed1b0ff8427d6580adcecc
fweikert pushed a commit to fweikert/bazel that referenced this issue May 25, 2023
With the deletion of the Java rule implementation, large amounts of code are
now unused. Entire removal of the classes isn't yet possible, but most of
their code can be removed.

This also makes it easier to transition to the Starlark providers, since less
Java code is referencing the to-be-deleted Java providers

Work towards bazelbuild#15897

PiperOrigin-RevId: 514566571
Change-Id: I28ef124369fc678945873bc21919830cb73eafc8
fweikert pushed a commit to fweikert/bazel that referenced this issue May 25, 2023
The Java rule implementation of them has been deleted.

Work towards bazelbuild#15897

PiperOrigin-RevId: 514769735
Change-Id: Ie7fa8e0f9d51943547319c6514edf33f8c47299c
fweikert pushed a commit to fweikert/bazel that referenced this issue May 25, 2023
Because providers are based on identity, and there are Java tests that need
to reference the provider, it's hard to split up the changes, so it's all
in a single change.

The Java class is kept to make usage by the Java tests easier; it isn't
actually used outside of tests.

Work towards bazelbuild#15897

PiperOrigin-RevId: 523182610
Change-Id: I6e07cf55ba08bb87c5ccb6cd37b473ed6aeeb760
fweikert pushed a commit to fweikert/bazel that referenced this issue May 25, 2023
Because providers are based on identity, and there are Java tests that need
to reference the provider, it's hard to split up the changes, so it's all
in a single change.

The Java class is kept to make usage by the Java tests easier; it isn't
actually used outside of tests.

Work towards bazelbuild#15897

PiperOrigin-RevId: 523748496
Change-Id: I1f5e9316bb3c4aed6c36fbd317914e4deea26f61
fweikert pushed a commit to fweikert/bazel that referenced this issue May 25, 2023
Because providers are based on identity, and there are Java tests that need
to reference the provider, it's hard to split up the changes, so it's all
in a single change.

A side-effect of this is the name used to access the provider in `cquery` commands
changes slightly: from "PyInfo" to "@_builtins//:common/python/providers.bzl%PyInfo". This fully qualified name is considered internal and shouldn't be relied upon; it will change again when
the rules are removed from Bazel and part of rules_python.

The Java class is kept to make usage by the Java tests easier; it isn't
actually used outside of tests.

Work towards bazelbuild#15897

PiperOrigin-RevId: 524054712
Change-Id: I1ab7d299c7b0458e0ee5359babb1fa63f9739498
lripoche pushed a commit to lripoche/bazel that referenced this issue Jun 13, 2023
The implementation is still a no-op.

Work towards bazelbuild#15897

PiperOrigin-RevId: 483711468
Change-Id: Ia13d7ceeba7a3a2bc1e416fe94f46ac5f7ee0f35
lripoche pushed a commit to lripoche/bazel that referenced this issue Jun 13, 2023
Work towards bazelbuild#15897

PiperOrigin-RevId: 483722548
Change-Id: Ieee7859e516a946286eeb07f9a86d1ea2b159aa0
lripoche pushed a commit to lripoche/bazel that referenced this issue Jun 13, 2023
Various fixes from the rushed initial implementation
 * ctx.attr.files is a list of targets, not a single target
 * interpreter_path defaults to an empty string when not specified, while
   it must be None for PyRuntimeInfo to accept the non-hermetic case.
 * The files arg must be None for the non-hermetic case (an empty depset is
   not accepted by PyRuntimeInfo).
 * Missing the `.py` part when access the fragment to get the default
   Python version.
 * Default `python_version` to `_INTERNAL_SENTINEL` to match existing
   behavior (otherwise it defaults to empty string)

Work towards bazelbuild#15897

PiperOrigin-RevId: 485109694
Change-Id: Icfbbf606acb8074c12e7dba9b3ec39e6ac524bc5
lripoche pushed a commit to lripoche/bazel that referenced this issue Jun 13, 2023
…tarlark to use Java helpers

* `py_builtins` is an Starlark builtins-internal-only namespace for functions
  that only builtins should use.
* `py_internal` is for regular Starlark for testing and other esoteric
   purposes. Usage of this is restricted to packages relating to the rule
   implementations to prevent using these internal APIs.

(These already exist in the Google-internal build; this makes them defined
in Bazel, too, though they're currently no-ops; followup CLs will begin moving
common code out of the Google-py_internal and into the common one).

Work towards bazelbuild#15897

PiperOrigin-RevId: 485146419
Change-Id: I71902e60471a4227c1993778bda60cbbe6e316f0
lripoche pushed a commit to lripoche/bazel that referenced this issue Jun 13, 2023
…rage tool.

Work towards bazelbuild#15897

PiperOrigin-RevId: 485161913
Change-Id: I65128d9371a3e2124b2a82143530d88ba23e49b7
lripoche pushed a commit to lripoche/bazel that referenced this issue Jun 13, 2023
This will be necessary to implement `py_runtime` in regular Starlark, so
expose it now so it's available later.

Work towards bazelbuild#15897

PiperOrigin-RevId: 485180942
Change-Id: I9b79b9d3938de80307a94b6db37569c0a577f8d6
lripoche pushed a commit to lripoche/bazel that referenced this issue Jun 13, 2023
I manually built a Bazel and ran tests in rules_python with it; looks to be OK.

Work towards bazelbuild#15897

PiperOrigin-RevId: 485978566
Change-Id: I79c209b095492c39b1fff97091425fc7af486b12
lripoche pushed a commit to lripoche/bazel that referenced this issue Jun 13, 2023
This is a basic transcription of the PyRuntimeInfo class; it isn't enabled, used,
or tested yet. Subsequent changes will address test failures.

Work towards bazelbuild#15897

PiperOrigin-RevId: 485988691
Change-Id: Ic42b03bc6b82d04742273b6a7f331665dd91aee5
tpudlik added a commit to tpudlik/emboss that referenced this issue Apr 10, 2024
This adds an explicit dependency on rules_python and loads py_library
and py_binary rules from there, instead of relying on built-in versions
in Bazel.

This prepares us for the rollout of
bazelbuild/bazel#15897 (which is happening
internally already, and across the Bazel ecosystem soon).

Except for the change to the WORKSPACE file, this change was
auto-generated by running,

    buildozer 'new_load @rules_python//python:py_library.bzl py_library' //...:__pkg__
    buildozer 'new_load @rules_python//python:py_binary.bzl py_binary' //...:__pkg__
    buildozer 'new_load @rules_python//python:py_test.bzl py_test' //...:__pkg__
    buildozer 'fix unusedLoads' //...:__pkg__

Upstreams cl/551628592.
tpudlik added a commit to tpudlik/emboss that referenced this issue Apr 10, 2024
This adds an explicit dependency on rules_python and loads py_library
and py_binary rules from there, instead of relying on built-in versions
in Bazel.

This prepares us for the rollout of
bazelbuild/bazel#15897 (which is happening
internally already, and across the Bazel ecosystem soon).

Except for the change to the WORKSPACE file, this change was
auto-generated by running,

    buildozer 'new_load @rules_python//python:py_library.bzl py_library' //...:__pkg__
    buildozer 'new_load @rules_python//python:py_binary.bzl py_binary' //...:__pkg__
    buildozer 'new_load @rules_python//python:py_test.bzl py_test' //...:__pkg__
    buildozer 'fix unusedLoads' //...:__pkg__

Upstreams cl/551628592.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Rules-Python Native rules for Python type: bug
Projects
None yet
Development

No branches or pull requests

5 participants