Allow `extry_point` to include extra targets #980

xinbinhuang · 2023-01-11T16:39:16Z

PR Checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature (please, look at the "Scope of the project" section in the README.md file)
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
Other... Please describe:

What is the current behavior?

Issue Number: #979

alias(
    name = "flake8",
    actual = entry_point(pkg="flake8", script="flake8")
)

What is the new behavior?

entry_point(
    name = "flake8",
    pkg = "flake8",
    script="flake8",
    deps = [...]        # any other kwargs applicable to py_binary
)

Does this PR introduce a breaking change?

Yes
No

Other information

xinbinhuang · 2023-01-13T08:18:51Z

r? @alexeagle would you like to take a look since you approve/merge the first version of entry_point?

rickeylev · 2023-01-18T22:33:55Z

examples/pip_parse/BUILD.bazel

+entry_point_binary(
+    name = "yamllint_binary",
+    pkg = "yamllint",
+)


Since the basic feature is to allow additional dependencies, please provide an example where it does so. Otherwise it isn't clear what the difference is between these two, and its harder for readers to see code reflective of their case.

rickeylev · 2023-01-18T22:36:01Z

python/pip.bzl

@@ -235,6 +235,9 @@ def dist_info_requirement(name):
 def entry_point(pkg, script = None):
    fail("Not implemented yet")

+def entry_point_binary(name, pkg, script = None, **kwargs):
+    fail("Not implemented yet")


Instead of simply "not implemented yet", lets change the message to better indicate that codegen generates the implementation for this, so if you hit this, it means you're loading the wrong thing.

alexeagle · 2023-01-18T22:42:51Z

It feels to me like the entry_point is syntax sugar for the simple case, and when you have additional dependencies you can easily write your own py_binary to run the tool with those dependencies (as you did for every package before entry_point was introduced). Also rules_js does a similar generation of convenience binaries for 3p packages, and doesn't have a feature like this.

So IMO this isn't needed and adds maintenance burden for rules_python.

xinbinhuang · 2023-01-20T18:36:22Z

It feels to me like the entry_point is syntax sugar for the simple case, and when you have additional dependencies you can easily write your own py_binary to run the tool with those dependencies (as you did for every package before entry_point was introduced).

You're totally right that it's a syntax sugar and user can build their own to involve additional dependencies. However, from a user perspective, it's not obvious at first glance on how to do that, especially for users not familiar with Bazel and rules_python. Also, it seems a bit awkward to provide a half-functioned solution that indicate to the users that it'll work, but in fact with significant caveats.

If we keep the current solution, at least we should clearly call out the limitation and list out the alternatives in the documentation so users don't get confused on this. However, I would argue that i'll be better time spent to just provide a better solution instead of writing those documentation.

Also rules_js does a similar generation of convenience binaries for 3p packages, and doesn't have a feature like this.

I think rules_js already has this feature (examples). Actually, rules_js's features inspire me on this PR. Or are you referring to something else?

So IMO this isn't needed and adds maintenance burden for rules_python.

I think the end goal here is to only keep the new entry_point_binary and remove the old alias later. My original plan was to augmenet/modify the existing entry_point, but it seems a bit akward given how the current one is used (I'm happy to expand on what this means if you feel is needed). So I introduced the entry_point_binary to avoid breaking changes and ease for user migration - similar to what is done for pip_parse and pip_install.

groodt · 2023-01-20T18:48:00Z

The maintainers have discussed various plans to redesign how entry_points and the various generated functions are handled. We’re going to try move away from such a heavy reliance on generated repository rules. The functionality described in this PR would work with simple macros too (and would probably be more appropriate since it’s syntax sugar).

xinbinhuang · 2023-01-20T19:11:09Z

Hey @groodt, thx for the context. One quick question

PR would work with simple macros too

Do you mean by a macro provided by rules_python to extract entry point from the package? Curious on what the timeline would be.

xinbinhuang · 2023-01-26T05:44:32Z

@groodt @alexeagle @rickeylev hey, just wanna check to see if we still want to have this PR merged in some way. Or if we want to take a totally different approach, that's ok too. Let me know how you think.

github-actions · 2023-07-25T22:50:43Z

This Pull Request has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days.
Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

rafmagns-skepa-dreag · 2023-08-01T20:45:38Z

PR author asked questions that were never answered on publicly github. would like to see them answered before this PR is auto-closed

Introduce a new `entry_point` macro to `rules_python` as opposed to the `hub` repository which allows users to generate an `entry_point` script for a given package. This will check the `console_scripts` key in the `entry_points.txt` dist-info file and avoids eager fetching of third party repositories because it is a `genrule` and a `py_binary` underneath the hood and exists in `rules_python`. This is a breaking change for bzlmod users as they will have to start using `@rules_python//python:entry_point.bzl`. For others this new macro is available to be used, but the old code is still present. Fixes bazelbuild#1362 Fixes bazelbuild#543 Fixes bazelbuild#979 Fixes bazelbuild#1262 Closes bazelbuild#980 Closes bazelbuild#1294

rickeylev · 2023-08-18T20:26:09Z

Over in #979 we're talking about and spec'ing out a new rule/macro to handle this case. There's a pending PR with most of the functionality implemented.

At this point, we're just finishing up some details and waiting for the PR author to come back from vacation. As such, I'll close this PR.

Introduce a new `entry_point` macro to `rules_python` as opposed to the `hub` repository which allows users to generate an `entry_point` script for a given package. This will check the `console_scripts` key in the `entry_points.txt` dist-info file and avoids eager fetching of third party repositories because it is a `genrule` and a `py_binary` underneath the hood and exists in `rules_python`. This is a breaking change for bzlmod users as they will have to start using `@rules_python//python:entry_point.bzl`. For others this new macro is available to be used, but the old code is still present. Fixes bazelbuild#1362 Fixes bazelbuild#543 Fixes bazelbuild#979 Fixes bazelbuild#1262 Closes bazelbuild#980 Closes bazelbuild#1294

#1363) Add `py_console_script_binary`, a macro/rule that allows better customization of how entry points are generated. Notable features of it are: * It allows passing in additional dependencies, which makes it easier for plugin dependencies to be added to tools such as pylint or sphinx. * The underlying `py_binary` rule can be passed in, allowing custom rules, such as the version-aware rules, to be used for the resulting binary. * Entry point generation is based upon a wheel's `entry_points.txt` file. This helps avoid loading external repositories unless they're actually used, allows entry points to have better version-aware support, and allows bzlmod to provide a supportable mechanism for entry points. Because the expected common use case is an entry point for our pip generated repos, there is special logic to make that easy and concisely do. Usage of `py_console_script_binary` is not tied to our pip code generation, though, and users can manually specify dependencies if they need to. BREAKING CHANGE: This is a breaking change, but only for bzlmod users. Note that bzlmod support is still beta. Bzlmod users will need to replace using `entry_point` from `requirements.bzl` with loading `py_console_script_binary` and defining the entry point locally: ``` load("@rules_python//python/entry_points:py_console_script_binary.bzl, "py_console_script_binary") py_console_script_binary(name="foo", pkg="@mypip//pylint") ``` For workspace users, this new macro is available to be used, but the old code is still present. Fixes #1362 Fixes #543 Fixes #979 Fixes #1262 Closes #980 Closes #1294 Closes #1055 --------- Co-authored-by: Richard Levasseur <richardlev@gmail.com>

xinbinhuang requested a review from hrfuller as a code owner January 11, 2023 16:39

xinbinhuang changed the title ~~Try refactor entry_point~~ Allow extra_point to include extra dependencies Jan 11, 2023

xinbinhuang changed the title ~~Allow extra_point to include extra dependencies~~ Allow extra_point to include extra targets Jan 11, 2023

xinbinhuang changed the title ~~Allow extra_point to include extra targets~~ [WIP] Allow extra_point to include extra targets Jan 11, 2023

xinbinhuang requested a review from rickeylev as a code owner January 11, 2023 19:11

xinbinhuang force-pushed the add_deps_entry_point branch 5 times, most recently from 071c9c0 to 3455b2f Compare January 12, 2023 10:46

Add entry_point_binary

9ec07b4

xinbinhuang force-pushed the add_deps_entry_point branch from 3455b2f to 9ec07b4 Compare January 12, 2023 10:52

xinbinhuang added 2 commits January 12, 2023 04:18

fixup! Add entry_point_binary

3334366

fixup! fixup! Add entry_point_binary

100a19c

xinbinhuang changed the title ~~[WIP] Allow extra_point to include extra targets~~ Allow extra_point to include extra targets Jan 12, 2023

rickeylev changed the title ~~Allow extra_point to include extra targets~~ Allow extry_point to include extra targets Jan 18, 2023

rickeylev requested changes Jan 18, 2023

View reviewed changes

github-actions bot added the Can Close? Will close in 30 days if there is no new activity label Jul 25, 2023

github-actions bot removed the Can Close? Will close in 30 days if there is no new activity label Aug 1, 2023

This was referenced Aug 3, 2023

Include extra dependencies for entry_point #979

Closed

feat(py_console_script_binary)!: entry points with custom dependencies #1363

Merged

rickeylev closed this Aug 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow `extry_point` to include extra targets #980

Allow `extry_point` to include extra targets #980

xinbinhuang commented Jan 11, 2023 •

edited

Loading

xinbinhuang commented Jan 13, 2023 •

edited

Loading

rickeylev Jan 18, 2023

rickeylev Jan 18, 2023

alexeagle commented Jan 18, 2023

xinbinhuang commented Jan 20, 2023 •

edited

Loading

groodt commented Jan 20, 2023

xinbinhuang commented Jan 20, 2023 •

edited

Loading

xinbinhuang commented Jan 26, 2023

github-actions bot commented Jul 25, 2023

rafmagns-skepa-dreag commented Aug 1, 2023

rickeylev commented Aug 18, 2023

Allow extry_point to include extra targets #980

Allow extry_point to include extra targets #980

Conversation

xinbinhuang commented Jan 11, 2023 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

xinbinhuang commented Jan 13, 2023 • edited Loading

rickeylev Jan 18, 2023

Choose a reason for hiding this comment

rickeylev Jan 18, 2023

Choose a reason for hiding this comment

alexeagle commented Jan 18, 2023

xinbinhuang commented Jan 20, 2023 • edited Loading

groodt commented Jan 20, 2023

xinbinhuang commented Jan 20, 2023 • edited Loading

xinbinhuang commented Jan 26, 2023

github-actions bot commented Jul 25, 2023

rafmagns-skepa-dreag commented Aug 1, 2023

rickeylev commented Aug 18, 2023

Allow `extry_point` to include extra targets #980

Allow `extry_point` to include extra targets #980

xinbinhuang commented Jan 11, 2023 •

edited

Loading

xinbinhuang commented Jan 13, 2023 •

edited

Loading

xinbinhuang commented Jan 20, 2023 •

edited

Loading

xinbinhuang commented Jan 20, 2023 •

edited

Loading