-
-
Notifications
You must be signed in to change notification settings - Fork 640
feat(pypi): enable pipstar by default #3225
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
feat(pypi): enable pipstar by default #3225
Conversation
|
Need to wait for the #3247 so that I can finish fixing the tests |
b697f55 to
790a508
Compare
790a508 to
7b35e52
Compare
Extra fixes: - add more logging to the failure - better METADATA files in tests - fix tests
7b35e52 to
10692ce
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request enables pipstar by default for bzlmod builds, which is a significant step forward. The changes in internal_config_repo.bzl correctly implement this switch. The test updates, including modifications to hub_builder_tests.bzl and test data, align well with this new default, ensuring the test suite remains valid. The improved error logging in whl_metadata.bzl is also a nice addition for debuggability. I've found one potential issue regarding the use of dep_template in test dependencies which could cause problems in the future.
|
OK, PTAL. |
3610512 to
a61e676
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request enables the Starlark-based pipstar dependency resolver by default, a significant step towards moving dependency evaluation to the analysis phase. The changes include extensive plumbing to support this, such as introducing whl_config_repository to replace group_library, and passing package information via a config.bzl file. The experimental_target_platforms attribute is now deprecated. The implementation is robust, with considerations for backward compatibility, and the test suite has been thoroughly updated to reflect these substantial changes. My review found the changes to be well-executed, with only a minor suggestion to improve the clarity of the deprecation notice in the changelog.
aignas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self review.
|
PTAL |
66ed60e to
42fef58
Compare
rickeylev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any known compatibility issues we should document, or should this be a transparent drop in replacement?
|
This should be a drop-in replacement, oh I think we need to point users to how they can configure the environment used for doing the evaluation. |
Followup to #3225 to add more documentation.
Before this PR we were using our python helpers to parse the whl METADATA for a
particular host (or experimentally - target) platform and the fixed dependency
list would be written to the
BUILD.bazelfiles materialized by thewhl_libraryrepository rule.This PR adds extra plumbing to leverage the Starlark implementation of the
METADATA file parsing (a.k.a. pipstar) which moves the evaluation to analysis
phase as we will get the target platform details via the
env_marker_config_settingfrom thetoolchainused for the dependencies.Since users are normally working with a subset of platforms that the METADATA
would pull in, we are writing the list of packages from the requirements file to
a
bzlfile so thatpipstarknows which packages to consider when parsing theMETADATA. This will ensure that
bazel querycontinues to work.Since just passing the list of packages to the
whl_librarywould causerefetches of all of the
whl_librarydependencies when we add or remove apackage, we pass a path where to load the symbol called
packagesfrom, whichmeans that only the analysis phase will be affected because the macros will be
re-evaluated if one adds or removes a package.
We still need to download the correct platform-specific wheels for the right
platform in order to fully resolve the referenced tickets. With this PR we are
deprecating the
experimental_target_platformsattribute since it has no longerany effect.
Fixes #2949
Work towards #260
Work towards #2241