Skip to content

Commit

Permalink
address rickeylev comments and general cleanup
Browse files Browse the repository at this point in the history
* Wrap changelog text
* Make comment about allow_files more descriptive
* Improve doc about interpreter attribute behavior
* use helper_target()
  • Loading branch information
rickeylev committed Jan 8, 2024
1 parent c60b21f commit 6af9d0c
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 14 deletions.
4 changes: 2 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ A brief description of the categories of changes:
* (pip_install) the deprecated `pip_install` macro and related items have been
removed.

* (toolchains) `py_runtime` can now take an executable target. Note: runfiles from
the target are not supported yet.
* (toolchains) `py_runtime` can now take an executable target. Note: runfiles
from the target are not supported yet.

### Fixed

Expand Down
28 changes: 21 additions & 7 deletions python/private/common/py_runtime_rule.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ def _py_runtime_impl(ctx):
runtime_files,
])
else:
fail("interpreter must be an executable target or must product exactly one file.")
fail("interpreter must be an executable target or must produce exactly one file.")

if ctx.attr.coverage_tool:
coverage_di = ctx.attr.coverage_tool[DefaultInfo]
Expand Down Expand Up @@ -204,14 +204,28 @@ runtime. For a platform runtime this attribute must not be set.
""",
),
"interpreter": attr.label(
# We set `allow_files = True` because users should have
# the ability to set this attr to an executable target, such as
# a target created by `sh_binary`
# We set `allow_files = True` to allow specifying executable
# targets from rules that have more than one default output,
# e.g. sh_binary.
allow_files = True,
doc = """
For an in-build runtime, this is the target to invoke as the interpreter. For a
platform runtime this attribute must not be set. WIP: If the target is executable
the runfiles may not be properly setup, see bazelbuild/rules_python/issues/1612
For an in-build runtime, this is the target to invoke as the interpreter. It
can be either of:
* A single file, which will be the interpreter binary. It's assumed such
interpreters are either self-contained single-file executables or any
supporting files are specified in `files`.
* An executable target. The target's executable will be the interpreter binary.
Any other default outputs (`target.files`) and plain files runfiles
(`runfiles.files`) will be automatically included as if specified in the
`files` attribute.
NOTE: the runfiles of the target may not yet be properly respected/propagated
to consumers of the toolchain/interpreter, see
bazelbuild/rules_python/issues/1612
For a platform runtime (i.e. `interpreter_path` being set) this attribute must
not be set.
""",
),
"interpreter_path": attr.string(doc = """
Expand Down
3 changes: 0 additions & 3 deletions tests/py_runtime/pretend_binary

This file was deleted.

5 changes: 3 additions & 2 deletions tests/py_runtime/py_runtime_tests.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -321,9 +321,10 @@ def _test_system_interpreter_must_be_absolute_impl(env, target):
_tests.append(_test_system_interpreter_must_be_absolute)

def _test_interpreter_sh_binary_target(name):
native.sh_binary(
rt_util.helper_target(
native.sh_binary,
name = "built_interpreter",
srcs = [":pretend_binary"],
srcs = ["pretend_binary"],
)

rt_util.helper_target(
Expand Down

0 comments on commit 6af9d0c

Please sign in to comment.