wheel: support for 'plugin' type entry_points #349

dhalperi · Aug 20, 2020

PR Checklist

Does not include precompiled binaries, eg. .par files. See CONTRIBUTING.md for info
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?

Feature (please, look at the "Scope of the project" section in the README.md file)

What is the current behavior?

Python wheels have a rich concept called entry_points. The most common use of these is the existing console_scripts support: command-line wrappers for Python code.

However, there are many more uses. Another common one is plugins -- the Python version of, say, Java @AutoService. This is how pytest finds modules on the PYTHONPATH.

This PR is something we've been using for a while to add our own plugins to the entry_points file; but the change is worth discussing.

In one sense, this is stupid. There's some interference between console_scripts and plugins -- in reality, the underlying structure is:

[group name, like console_scripts]
key1 = value1
key2 = value2
...

[g2]
key3 = v3
key4 = v4
...

...

So maybe this would be best as a single argument merging console_scripts in: {group: {k1: v1, k2: v2, ...}, ...}... if we wanted to make a breaking change.

But since console_scripts already existed, I added plugins separately. (I did not call it entry_points since it doesn't handle console_scripts).

(There are of course, other backwards-compat options like adding an entry_points arg that throws if console_scripts is present.)

Issue Number: not yet, but happy to file one if needed

What is the new behavior?

Wheels built by rules_python support entry_points other than console_scripts, hopefully without breaking existing console_scripts support.

Does this PR introduce a breaking change?

No - At least, I tried not to.

Other information

See https://packaging.python.org/specifications/entry-points/ and specifically https://packaging.python.org/guides/creating-and-discovering-plugins/#using-package-metadata

Sorry for not ticking all the boxes from jump - this has been sitting around a while and when I realized PRs are being reviewed again I thought it might be worth starting the discussion.

This change is

See https://packaging.python.org/guides/creating-and-discovering-plugins/\#using-package-metadata

experimental/python/wheel.bzl

pstradomski · Aug 23, 2020

experimental/rules_python/wheelmaker.py

@@ -157,12 +158,26 @@ def add_metadata(self, extra_headers, description, classifiers, requires,
        metadata += "\n"
        self.add_string(self.distinfo_path('METADATA'), metadata)

-    def add_entry_points(self, console_scripts):
+    def add_entry_points(self, console_scripts, plugins):


I'd change it so that the .bzl file just merges console_scripts and plugins, and wheelmaker just always sees only entry_points.

experimental/python/wheel.bzl

pstradomski · Aug 23, 2020

experimental/rules_python/wheelmaker.py

+                match = pattern.fullmatch(line)
+                if not match:
+                    raise ValueError('{line} is not a valid entry point'.format(line=line))
+                plugin_type = match.group(1).strip()


This can produce multiple groups with the same name. Even if that's allowed by the spec (I'm not sure?) it would be better to order the entries so that each group appears only once.

This did not happen before (at least from wheel.bzl) because there was only one entry allowed per group. As part of switching that to allow multiple entry points per group, rewrite this code from scratch. I believe all such concerns are now handled.

* deprecate console_scripts, update docs * rename plugins to entry_points, update docs * switch to string_list_dict to support multiple entries per group * sorting of groups and entries within groups for idempotence

This reverts commit 5d5a06d.

experimental/python/wheel.bzl

dhalperi · Aug 24, 2020

experimental/rules_python/wheelmaker.py

@@ -157,12 +158,26 @@ def add_metadata(self, extra_headers, description, classifiers, requires,
        metadata += "\n"
        self.add_string(self.distinfo_path('METADATA'), metadata)

-    def add_entry_points(self, console_scripts):
+    def add_entry_points(self, console_scripts, plugins):


dhalperi · Aug 24, 2020

experimental/rules_python/wheelmaker.py

+                match = pattern.fullmatch(line)
+                if not match:
+                    raise ValueError('{line} is not a valid entry point'.format(line=line))
+                plugin_type = match.group(1).strip()


This did not happen before (at least from wheel.bzl) because there was only one entry allowed per group. As part of switching that to allow multiple entry points per group, rewrite this code from scratch. I believe all such concerns are now handled.

experimental/rules_python/wheelmaker.py

dhalperi · Aug 24, 2020

experimental/rules_python/wheelmaker.py

+            name = match.group(2).strip()
+            object_reference = match.group(3).strip()
+            if name in group_dict:
+                raise ValueError("Duplicate entry for name {name} in group {group}".format(name=name, group=group))


Is there a stock formatter for this repo? I tried yapf but it changed way more than just the modified code.

pstradomski

@andyscott @thundergolfer : This change looks good to me (except for the comments I've left). Should I merge it?

I authored the initial implementation of py_wheel for this repo and kept maintianing it, but with you as new maintainers I'd like a clarification if I should continue merging changes here or would you prefer to review/merge all new PRs?

experimental/python/wheel.bzl

pstradomski

Presubmit complains about the print() function.

Let's remove it.

pstradomski

Looks good to me

wheel: support for 'plugin' type entry_points

6f1df75

See https://packaging.python.org/guides/creating-and-discovering-plugins/\#using-package-metadata

dhalperi requested review from andyscott and thundergolfer as code owners August 20, 2020 19:01

googlebot added the cla: yes label Aug 20, 2020

Merge remote-tracking branch 'upstream/master' into for-pr

9b6c23e

pstradomski suggested changes Aug 23, 2020

View reviewed changes

dhalperi added 4 commits August 23, 2020 22:01

wheel_test.py: fix deprecation warning

5d5a06d

respond to review

8a1d960

* deprecate console_scripts, update docs * rename plugins to entry_points, update docs * switch to string_list_dict to support multiple entries per group * sorting of groups and entries within groups for idempotence

update comment

0aa64f4

Revert "wheel_test.py: fix deprecation warning"

c2a674f

This reverts commit 5d5a06d.

dhalperi commented Aug 24, 2020

View reviewed changes

dhalperi added 2 commits August 23, 2020 22:23

linter fix

840e477

update test

c01e596

pstradomski suggested changes Aug 24, 2020

View reviewed changes

experimental/python/wheel.bzl Show resolved Hide resolved

experimental/python/wheel.bzl Outdated Show resolved Hide resolved

review feedback

0ba59ed

dhalperi requested a review from pstradomski August 24, 2020 20:45

pstradomski suggested changes Aug 29, 2020

View reviewed changes

Remove print

dbe072f

dhalperi requested a review from pstradomski August 30, 2020 18:26

pstradomski approved these changes Aug 31, 2020

View reviewed changes

Merge branch 'master' into for-pr

6fc3d84

pstradomski merged commit a8d133a into bazel-contrib:master Aug 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

wheel: support for 'plugin' type entry_points #349

wheel: support for 'plugin' type entry_points #349

Uh oh!

dhalperi commented Aug 20, 2020 •

edited

Loading

Uh oh!

Uh oh!

pstradomski Aug 23, 2020

Uh oh!

dhalperi Aug 24, 2020

Uh oh!

Uh oh!

Uh oh!

pstradomski Aug 23, 2020

Uh oh!

dhalperi Aug 24, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dhalperi Aug 24, 2020

Uh oh!

dhalperi Aug 24, 2020

Uh oh!

Uh oh!

dhalperi Aug 24, 2020

Uh oh!

pstradomski left a comment

Uh oh!

Uh oh!

Uh oh!

pstradomski left a comment

Uh oh!

pstradomski left a comment

Uh oh!

Uh oh!

Search code, repositories, users, issues, pull requests...

Uh oh!

wheel: support for 'plugin' type entry_points #349

wheel: support for 'plugin' type entry_points #349

Uh oh!

Conversation

dhalperi commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

Uh oh!

pstradomski Aug 23, 2020

Choose a reason for hiding this comment

Uh oh!

dhalperi Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pstradomski Aug 23, 2020

Choose a reason for hiding this comment

Uh oh!

dhalperi Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dhalperi Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

dhalperi Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dhalperi Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

pstradomski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pstradomski left a comment

Choose a reason for hiding this comment

Uh oh!

pstradomski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dhalperi commented Aug 20, 2020 •

edited

Loading