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

refactor(pypi): move config setting processing to the macro #2424

Merged
merged 10 commits into from
Nov 22, 2024

Conversation

aignas
Copy link
Collaborator

@aignas aignas commented Nov 18, 2024

Before hand we would pass around whl_alias struct instances and it
would go through rounds of starlark functions which would end up
rendered as BUILD.bazel files in the hub repository. This means that
every time the output of the said functions would change, the
MODULE.bazel.lock would also possibly change.

Since we now have much better unit testing framework, we start relying
on these functions being evaluated at loading phase instead of repo rule
phase, which means that we can make the BUILD.bazel files not change a
lot when the underlying code changes, this should make the code more
maintainable in the long run and unblocks the code to accept extra
things, like env marker values or similar.

Summary:

  • Remove the repo field from whl_alias.
  • Rename whl_alias to whl_config_setting
  • Move the tests around
  • Simplify the pkg_aliases tests to use contains instead of exact
    equality to make things less brittle.
  • Simplify the pkg_aliases tests to use different mocks to make
    expectations easier to understand.
  • Make whl_config_setting hashable by using tuples instead of lists.
  • Adjust how we store the whl_config_setting in the PyPI extension
    and optimize the passing to the hub_repository.

This is needed for #2319 and #2423.

To be in future PRs:

  • Remove the need to pass osx_versions, etc to the pkg_aliases macro.

Before hand we would pass around `whl_alias` struct instances and it
would go through rounds of starlark functions which would end up
rendered as BUILD.bazel files in the hub repository. This means that
every time the output of the said functions would change, the
`MODULE.bazel.lock` would also possibly change.

Since we now have much better unit testing framework, we start relying
on these functions being evaluated at loading phase instead of repo rule
phase, which means that we can make the `BUILD.bazel` files not change a
lot when the underlying code changes, this should make the code more
maintainable in the long run and unblocks the code to accept extra
things, like env marker values or similar.

Summary:
- Remove the `repo` field from `whl_alias`.
- Rename `whl_alias` to `whl_config_setting`
- Move the tests around
- Simplify the `pkg_aliases` tests to use `contains` instead of exact
  equality to make things less brittle.
- Simplify the `pkg_aliases` tests to use different mocks to make
  expectations easier to understand.
- Make `whl_config_setting` hashable by using tuples instead of lists.

This is definitely needed for bazelbuild#2319 and bazelbuild#2423.
@aignas aignas force-pushed the refactor/use-whl-alias-in-hub branch from ae80da5 to 16cf803 Compare November 18, 2024 09:32
@aignas aignas marked this pull request as ready for review November 18, 2024 10:11
python/private/pypi/hub_repository.bzl Outdated Show resolved Hide resolved
python/private/pypi/pkg_aliases.bzl Outdated Show resolved Hide resolved
python/private/pypi/pkg_aliases.bzl Outdated Show resolved Hide resolved
python/private/pypi/pkg_aliases.bzl Outdated Show resolved Hide resolved
python/private/pypi/render_pkg_aliases.bzl Outdated Show resolved Hide resolved
@aignas
Copy link
Collaborator Author

aignas commented Nov 22, 2024

I need to revisit the error message wording here. Will do later.

@aignas aignas enabled auto-merge November 22, 2024 04:12
@aignas aignas added this pull request to the merge queue Nov 22, 2024
Merged via the queue into bazelbuild:main with commit 2abca35 Nov 22, 2024
4 checks passed
ewianda pushed a commit to ewianda/rules_python that referenced this pull request Nov 22, 2024
…ld#2424)

Before hand we would pass around `whl_alias` struct instances and it
would go through rounds of starlark functions which would end up
rendered as BUILD.bazel files in the hub repository. This means that
every time the output of the said functions would change, the
`MODULE.bazel.lock` would also possibly change.

Since we now have much better unit testing framework, we start relying
on these functions being evaluated at loading phase instead of repo rule
phase, which means that we can make the `BUILD.bazel` files not change a
lot when the underlying code changes, this should make the code more
maintainable in the long run and unblocks the code to accept extra
things, like env marker values or similar.

Summary:
- Remove the `repo` field from `whl_alias`.
- Rename `whl_alias` to `whl_config_setting`
- Move the tests around
- Simplify the `pkg_aliases` tests to use `contains` instead of exact
  equality to make things less brittle.
- Simplify the `pkg_aliases` tests to use different mocks to make
  expectations easier to understand.
- Make `whl_config_setting` hashable by using tuples instead of lists.
- Adjust how we store the `whl_config_setting` in the PyPI extension
  and optimize the passing to the `hub_repository`.

This is needed for bazelbuild#2319 and bazelbuild#2423.

To be in future PRs:
* Remove the need to pass `osx_versions`, etc to the `pkg_aliases`
macro.
aignas added a commit to aignas/rules_python that referenced this pull request Nov 27, 2024
…ms set

It seems that during bazelbuild#2424 I broke the rendering of aliases for the
cases when the target platform is set. This means that the feature for
multiplatform whls when `experimental_index_url` has never worked even
though it was advertised. This ensures that the rendering is happening
correctly and adds extra missing tests.

Whilst at it:
- add an extra test for `pip.parse` handling of env markers that I added
  to ensure that the error is not in the module extension.
- Cleanup unused code - error message constant and the repo arg in
  `whl_config_setting`.

Fixes bazelbuild#2446
rickeylev pushed a commit that referenced this pull request Nov 27, 2024
…ms set (#2447)

It seems that during #2424 I broke the rendering of aliases for the
cases when the target platform is set. This means that the feature for
multiplatform whls when `experimental_index_url` has never worked even
though it was advertised. This ensures that the rendering is happening
correctly and adds extra missing tests.

Whilst at it:
- add an extra test for `pip.parse` handling of env markers that I added
  to ensure that the error is not in the module extension.
- Cleanup unused code - error message constant and the repo arg in
  `whl_config_setting`.

Fixes #2446
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants