feat(py_wheel): Normalize name and version #1331

vonschultz · 2023-07-20T11:17:19Z

Added the incompatible_normalize_name feature flag to normalize the package distribution name according to latest Python packaging standards. Defaults to False for the time being.

Added the incompatible_normalize_version feature flag to normalize the package version according to PEP440 standard. This also adds support for local version specifiers (versions with a + in them), in accordance with PEP440. Defaults to False for the time being.

Instead of following the obsolete PEP 427 escaping procedure for distribution names and versions, we should use the rules specified by https://packaging.python.org/en/latest/specifications (sections "Package name normalization" and "Binary distribution format"). For the versions, this means normalizing them according to PEP 440.

Added as feature flags to avoid forcing the user to deal with breaking changes when upgrading rules_python:

Distribution names have stronger requirements now: "A valid name consists only of ASCII letters and numbers, period, underscore and hyphen. It must start and end with a letter or number." https://packaging.python.org/en/latest/specifications/name-normalization/
Versions must be valid PEP 440 version identifiers. Previously versions such as "0.1-2-3" would have been accepted; that is no longer the case.
The file name of generated wheels may have changed, if the distribution name or the version identifier wasn't in normalized form.
The wheelmaker now depends on packaging.version, which means the py_wheel user now needs to load pip dependencies in their WORKSPACE.bazel file:
```
load("@rules_python//python/pip_install:repositories.bzl", "pip_install_dependencies")

pip_install_dependencies()
```

Fixes #883. Fixes #1132.

Instead of following the obsolete PEP 427 escaping procedure for distribution names and versions, use the rules specified by https://packaging.python.org/en/latest/specifications (sections "Package name normalization" and "Binary distribution format"). For the versions, this means normalizing them according to PEP 440. This adds full support for PEP 440-compliant version identifiers, including local version identifiers (the part after "+" in versions such as "1.0+ubuntu.1"). BREAKING CHANGE: - Distribution names have stronger requirements now: "A valid name consists only of ASCII letters and numbers, period, underscore and hyphen. It must start and end with a letter or number." https://packaging.python.org/en/latest/specifications/name-normalization/ - Versions must be valid PEP 440 version identifiers. Previously versions such as "0.1-2-3" would have been accepted; that is no longer the case. - The file name of generated wheels may have changed, if the distribution name or the version identifier wasn't in normalized form. Fixes bazel-contrib#883

vonschultz · 2023-07-20T11:36:56Z

Could I get some help with the test failures? They seem to be related to this line:
https://github.com/bazelbuild/rules_python/blob/5c5ab5bd9577a284784d1c8b27bf58336de06010/python/runfiles/BUILD.bazel#L55
where, of course, {BUILD_EMBED_LABEL} is not a valid version according to PEP 440. Now, I have been assuming that once we get to wheelmaker.py any stamping should already have taken place, taking care of all the placeholders, and the version we get should be a valid PEP 440 version. Is this mistaken? Should wheelmaker.py be forced to handle invalid versions with placeholders in them? Or should the {BUILD_EMBED_LABEL} be replaced with a valid version identifier?

Looks like @alexeagle and @rickeylev were involved in the patch (1722988c) that introduced {BUILD_EMBED_LABEL}. Any insights or opinions?

aignas · 2023-07-24T07:02:57Z

I think by default we are not stamping the builds within the CI, so the embed-label is not substituted. This would mean that a restrictive version name validation may need to be conditional based on whether the build is being stamped.

Another approach could be to do --stamp --embed-label 0.0.0 for the affected tests, but I am not sure if this is the right approach, but usage of some escape latch might be necessary here because of the fact that this wheel is being built within the rules_python workspace.

chrislovecnm · 2023-08-07T18:33:41Z

@vonschultz does this change fix this issue: #1132

alexeagle · 2023-08-07T18:57:42Z

Even in an unstamped build, version placeholders should be replaced by a constant value like 0.0.0-PLACEHOLDER. IMO it was always incorrect that we left literal {BUILD_EMBED_LABEL} segments in output filenames.

We need a valid PEP 440 version even if we're doing a non-stamped build, even if there are placeholders (stamping keys) in the version string. In that case, replace the placeholders with 0, and append the original version string, sanitized to dot-separated alphanumerics.

vonschultz · 2023-08-08T13:21:43Z

Yes, @chrislovecnm, this fixes #1132. I updated the pull request message accordingly.

chrislovecnm · 2023-08-08T14:46:23Z

@aignas or @rickeylev PTAL

rickeylev

re: requiring pep 440 compliant names: I don't think we should try to perform such strict validation. Doing the basic transforms to make a string more pep 440 compatible (e.g. - to . or w/e) makes sense because we want to produce output that other tools can consume. But we don't really care if a user does e.g. .after instead of .post, or does .post0.dev4.rc1.

The wheelmaker now depends on packaging.version, which means the py_wheel user now needs to load pip dependencies in their WORKSPACE.bazel file

I'm pretty sure we call pip_install_dependencies() internally, so users don't need to worry about this. You can remove this from the description.

tests/py_wheel/py_wheel_tests.bzl

rickeylev · 2023-08-10T23:42:23Z

python/private/py_wheel.bzl

+        )
+    return escaped
+
+def normalize_pep440(version):


Well, wow. Now this is some Starlark!

But, this is very hard to follow -- this looks like a stack-based parser, and there's several hundred lines of nested defs that close over local state and modify that same state.

This is going to take some effort to review, which doesn't bode well for others to figure out, and thus its maintainability. We'll need to change it so that it's more approachable to other devs. I'll give some more concrete feedback after I take a closer look.

It's a parser, yes. If you have ideas on how to make it more approachable, I'm all for that.

My preference would have been to use a regular expression instead — in particular because PEP 440 includes a regular expression that we could take more or less as it is. But regular expressions aren't supported in Starlark, so here I am writing a parser instead. (Though whether a regular expression is more approachable than a parser is perhaps debatable, unless you're really into regular expressions.)

My apologies for the complexity, but PEP 440 is hundreds of lines of English, so some of the complexity seems unavoidable. On the other hand, PEP 440 is a fixed document. Once we have a normalize_pep440() function that works to our satisfaction, there will be few (if any) reasons to go back and change it. That's good for maintainability, isn't it, functions that have no conceivable reason to change as the product evolves.

tests/py_wheel/py_wheel_tests.bzl

vonschultz · 2023-08-11T08:10:34Z

re: requiring pep 440 compliant names: I don't think we should try to perform such strict validation. Doing the basic transforms to make a string more pep 440 compatible (e.g. - to . or w/e) makes sense because we want to produce output that other tools can consume. But we don't really care if a user does e.g. .after instead of .post, or does .post0.dev4.rc1.

I don't think we in rules_python should be making up our own rules for how Python versions work. The authoritative source is the specification for the binary distribution format at https://packaging.python.org/en/latest/specifications/binary-distribution-format/, and it says "Version numbers should be normalised according to PEP 440." We might also look at how influential projects such as setuptools handle it, and they require PEP 440-conforming versions. https://setuptools.pypa.io/en/latest/history.html#v66-0-0

In my mind one of two approaches makes sense. Either we take it upon us to do the normalization, in which case we should do it according to the specification, or we put the responsibility on the user entirely, documenting that the version attribute of a py_wheel needs to be a version normalized according to PEP 440, and skip any validation and sanitation whatsoever. That would certainly make for a less complex implementation, but I'm not sure that's a good idea.

The wheelmaker now depends on packaging.version, which means the py_wheel user now needs to load pip dependencies in their WORKSPACE.bazel file

I'm pretty sure we call pip_install_dependencies() internally, so users don't need to worry about this. You can remove this from the description.

I tested this. If I just do a minimal WORKSPACE.bazel file,

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

http_archive(
    name = "rules_python",
    sha256 = "0a8003b044294d7840ac7d9d73eef05d6ceb682d7516781a4ec62eeb34702578",
    strip_prefix = "rules_python-0.24.0",
    url = "https://github.com/bazelbuild/rules_python/releases/download/0.24.0/rules_python-0.24.0.tar.gz",
)

load("@rules_python//python:repositories.bzl", "py_repositories")

py_repositories()

in a toy project, as documented on https://github.com/bazelbuild/rules_python/releases/tag/0.24.0, and then try to use a py_wheel it will work with the 0.24.0 release, and it won't work when running with bazel build --override_repository=rules_python=/home/von/src/github.com/vonschultz/rules_python ..., unless we change the toy project to include a call to pip_install_dependencies().

It's true that we call pip_install_dependencies() internally in rules_python, but the end user doesn't benefit from that, so from the point of view of the end user it's a breaking c 8000 hange.

alexmirrington · 2023-08-24T07:46:21Z

For wheel builds that don't require specific semantic versioning (e.g. for internal tools that are installed in multiple other projects at a company), I think it's pretty common to append a +sha suffix to the wheel version. Currently the validation doesn't support this, and it would be very helpful if it did as proposed in #883

edit: My apologies - was following along on #883 and #1132 but misplaced my comment. Super happy to see this happening! 🎉

vonschultz · 2023-08-24T11:02:27Z

For wheel builds that don't require specific semantic versioning (e.g. for internal tools that are installed in multiple other projects at a company), I think it's pretty common to append a +sha suffix to the wheel version. Currently the validation doesn't support this, and it would be very helpful if it did as proposed in #883

Not sure I follow @alexmirrington. What you are talking about is a local version identifier. That is not currently supported on the main branch, which is #883. Fixing #883 that is the purpose of this pull request, and the validation that's done here does support local version identifiers:
https://github.com/bazelbuild/rules_python/pull/1331/files#diff-e5abf313ad5bebb5c80d6984829326c40288f734daecb0217f5d13ef75adad23R600-R610

aignas

In general the parsing approach LGTM, but the tests could be improved to unit test parts of the parser and then sanity check that the parts work together. Right now we only have integration tests.

I have made some experiments in my branch and created a PR to this branch in vonschultz#1 so that it would be easy for you to review and merge.

@rickeylev, I agree that this looks a little bit daunting at first and may require a few iterations of cleanup whilst we get to understand the code, however, once the nested defs are removed and we have a stack-like parser with very few methods it suddenly becomes much easier to grok in my opinion.

tests/py_wheel/py_wheel_tests.bzl

python/private/py_wheel.bzl

aignas · 2023-08-27T03:31:53Z

python/private/py_wheel.bzl

+        token = version[context["start"]]
+
+        if predicate(token):
+            if type(value) in ["function", "builtin_function_or_method"]:


Since this is called many times, consider optimizing and doing

Suggested change

if type(value) in ["function", "builtin_function_or_method"]:

t = type(value)

if t == "function" or t == "builtin_function_or_method":

That way we are not creating a list just for comparison.

Are you sure? I'm not measuring any noticeable speed-up. Measuring with

time bazelisk test //tests/py_wheel:test_pep440_normalization

(with bazelisk clean in between), and sometimes the first approach is faster, sometimes the second approach is faster.

If it's speed we're after, it was noticeably faster before the readability improvements you suggested. (Sorry, I do like the readability improvements.) If we're going for readability over speed, I like the list comprehension.

Thanks for being rigorous and checking it. LGTM.

python/private/py_wheel.bzl

tools/wheelmaker.py

PEP440 normalization code structure suggestions

//python/private:py_wheel.bzl depends on a new file, py_wheel_normalize_pep440.bzl, which therefore needs to be exported by the python/private package and listed in the //docs:packaging_bzl sources.

In the case where the epoch parsed to "0!", it should be reset to the empty string. The previous way of doing that forgot to return acceptance to the caller; the new way of doing it is clearer than manipulating the "start" of the context.

The buildifier-lint checker wants arguments and return values to be documented.

The reviewers apparently like long lines. Not sold on that, but contributors should follow the style of the projects they contribute to.

aignas

Thanks for the PR, @rickeylev, with the new structure of having fewer things being passed via the closure and nested defs, does this look better?

aignas · 2023-09-20T23:20:52Z

python/private/py_wheel.bzl

-        _escape_filename_segment(ctx.attr.distribution),
-        _escape_filename_segment(version),
+        _escape_filename_distribution_name(ctx.attr.distribution),
+        normalize_pep440(version),


It would be great if we could make this new behaviour optional and gated under a boolean flag so that users that depend on the broken behaviour have more time to migrate.

The plan could be:

Add a flag named normalize_version_pep440 which defaults to False for now.

If the flag is set to True then the new codepath is used.

In a future release the incompatible_normalize_version is set to True but users have ability to set it to False if required.

That way the change is no longer breaking.

Consider adding an extra dictionary with args named:

_feature_flags = { "incompatible_normalize_version": attr.bool( default = False, doc = "Normalize the package version according to PEP440 standard", ), "incompatible_explicit_distribution_stamp_vars": attr.bool( default = False, doc = "Change the format of how the stamp variables can be used to " + "in the distribution name. With this option set to True, the " + "user has to pass stamp variables enclosed in '{}', e.g. " + "'{BUILD_TIMESTAMP}'." ), }

To add some context (I was out and about on my phone when Ignas asked me about this, so I didn't have time to respond in full):

The key point is to decouple upgrading the rules_python version from having to address incompatible changes. This allows a more incremental and manageable upgrade process. A user can upgrade rules_python, opt into behavior where they can, and followup later where they can't. The next version (which has the incompatible behavior by default) blocks them from upgrading, but they're still able to make their current version forward compatible.

examples/wheel/wheel_test.py

To avoid having breaking changes, add feature flags "incompatible_normalize_name" and "incompatible_normalize_version", which default to False for the time being.

Add the "incompatible_normalize_version" and "incompatible_normalize_name" feature flags to the documentation.

aignas

Thank you very much for doing the extra work to add the feature flags. I think the only thing remaining before merging is to add a line in the CHANGELOG.md file.

python/private/py_wheel.bzl

Add links to the specifications. Note that we have support for placeholders. Co-authored-by: Ignas Anikevicius <240938+aignas@users.noreply.github.com>

vonschultz · 2023-10-04T07:49:21Z

Should I update the pull request description? Does this still count as a breaking change? Does this still count as fixing #883 and #1132, or should that wait for a pull request that changes the default value of the feature flags to "True"?

Just a `bazelisk run //docs:update` after having changed the description of "incompatible_normalize_name".

aignas · 2023-10-04T23:51:29Z

Yes, the PR title and description should be updated and I would be of an opinion that it still fixes #883 and #1132 as users have a way to opt-into the new behavior.

Add a note about the two new feature flags in the change log: incompatible_normalize_name and incompatible_normalize_version.

vonschultz requested a review from rickeylev as a code owner July 20, 2023 11:17

rickeylev reviewed Aug 11, 2023

View reviewed changes

aignas added 17 commits August 27, 2023 10:00

refactor: move normalization into a separate file

0705d10

refactor: move helper nested defs to be private defs

d014bc6

refactor: move version and contexts to a self struct

1c79b26

refactor: create a normalize function

a9d06f0

refactor: move open_context to be a method on the normalizer struct

df2f124

refactor: move more predicates outside

1355341

refactor: move close_context to self.close_context

0a93c81

refactor: move accept to self.accept

9ed69be

refactor: move all of the methods to be functions instead

7f80890

refactor: restrict usage of contexts to the internal parser functions

f4be867

refactor: move accept back to be a regular function

efc66d4

refactor: rename context -> ctx

d2f6b74

refactor: rename tail -> context

03f6348

refactor: drop parameters from open_context

dca427c

refactor: rename pop -> pop_context

fcfff02

refactor: rename version -> input in the parser

ac27a62

refactor: rename pop_context -> discard

872a648

aignas added 2 commits August 27, 2023 12:50

refactor: rename close_context -> accept

4237439

6D40

refactor: use parser.discard and parser.accept methods

1076fd7

aignas reviewed Aug 27, 2023

View reviewed changes

vonschultz and others added 5 commits August 28, 2023 11:56

Merge pull request #1 from aignas/pep440-suggestions

6816bd4

PEP440 normalization code structure suggestions

Make pep440 file accessible by docs targets

2fb9457

//python/private:py_wheel.bzl depends on a new file, py_wheel_normalize_pep440.bzl, which therefore needs to be exported by the python/private package and listed in the //docs:packaging_bzl sources.

Fix branch with missing return in accept_epoch

2728e9f

In the case where the epoch parsed to "0!", it should be reset to the empty string. The previous way of doing that forgot to return acceptance to the caller; the new way of doing it is clearer than manipulating the "start" of the context.

Documentation fixes according to buildifier-lint

2e57719

The buildifier-lint checker wants arguments and return values to be documented.

Put load of normalize_pep440 on a single line

cd3f905

The reviewers apparently like long lines. Not sold on that, but contributors should follow the style of the projects they contribute to.

aignas approved these changes Aug 29, 2023

View reviewed changes

aignas requested changes Sep 20, 2023

View reviewed changes

vonschultz added 3 commits October 3, 2023 15:56

Add feature flags normalizing version or name

b2756dc

To avoid having breaking changes, add feature flags "incompatible_normalize_name" and "incompatible_normalize_version", which default to False for the time being.

Add feature flags to documentation

943be1b

Add the "incompatible_normalize_version" and "incompatible_normalize_name" feature flags to the documentation.

Merge remote-tracking branch 'origin/main' into pep440

74e8702

aignas reviewed Oct 3, 2023

View reviewed changes

python/private/py_wheel.bzl Outdated Show resolved Hide resolved

Update "incompatible_normalize_name" doc string

88c549b

Add links to the specifications. Note that we have support for placeholders. Co-authored-by: Ignas Anikevicius <240938+aignas@users.noreply.github.com>

Update feature flag description in documentation

ea41790

Just a `bazelisk run //docs:update` after having changed the description of "incompatible_normalize_name".

aignas added this to the v0.26.0 milestone Oct 5, 2023

Add an entry to the change log

08d555b

Add a note about the two new feature flags in the change log: incompatible_normalize_name and incompatible_normalize_version.

vonschultz force-pushed the pep440 branch from 55d4254 to 08d555b Compare October 5, 2023 13:00

vonschultz changed the title ~~feat(py_wheel)!: Normalize name and version~~ feat(py_wheel): Normalize name and version Oct 5, 2023

aignas approved these changes Oct 5, 2023

View reviewed changes

aignas added this pull request to the merge queue Oct 5, 2023

Merged via the queue into bazel-contrib:main with commit 382b678 Oct 5, 2023

vonschultz deleted the pep440 branch October 5, 2023 14:28

aignas mentioned this pull request Oct 19, 2023

Flip feature flags for 0.27.0 #1498

Closed

3 tasks

	if type(value) in ["function", "builtin_function_or_method"]:
	t = type(value)
	if t == "function" or t == "builtin_function_or_method":

Uh oh!

feat(py_wheel): Normalize name and version #1331

feat(py_wheel): Normalize name and version #1331

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!