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

fix(pip.parse): make the pip extension reproducible if PyPI is not called #1937

Merged
merged 2 commits into from
Jun 6, 2024

Conversation

aignas
Copy link
Collaborator

@aignas aignas commented Jun 2, 2024

With this PR we can finally have fewer lock file entries in setups which
do not use the experimental_index_url feature yet. This is fully
backwards compatible change as it relies on bazel doing the right
thing and regenerating the lock file.

Fixes #1643.

…lled

With this PR we can finally have fewer lock file entries in setups which
do not use the `experimental_index_url` feature yet. This is fully
backwards compatible change as it relies on `bazel` doing the right
thing and regenerating the lock file.

Fixes bazelbuild#1643.
@aignas aignas requested a review from rickeylev as a code owner June 2, 2024 16:00
@aignas
Copy link
Collaborator Author

aignas commented Jun 2, 2024

FYI @fmeum


if bazel_features.external_deps.extension_metadata_has_reproducible:
# We allow for calling the PyPI index and that will go into the MODULE.bazel.lock file
return module_ctx.extension_metadata(reproducible = False)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is equivalent to not returning metadata at all, so you could remove the entire block.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will do.

python/private/bzlmod/pip.bzl Outdated Show resolved Hide resolved
python/private/bzlmod/pip.bzl Outdated Show resolved Hide resolved
**_extension_extra_args()
)

pip_internal = module_extension(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to declare os/arch dependence.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily, since we are calling PyPI to get the right dependencies, the lock file will be the same on all platforms, so this can be removed, I'll change it so that it is the default behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@aignas aignas requested a review from groodt June 4, 2024 01:35
Copy link
Collaborator

@groodt groodt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aignas aignas added this pull request to the merge queue Jun 6, 2024
@fmeum
Copy link
Contributor

fmeum commented Jun 6, 2024

@aignas Would it be possible to get a release soon after this is merged? Bazel itself suffers from the per-os lockfile and having the extension marked as reproducible would greatly simplify the process of updating Bazel to build with Bazel 7.2.0.

Merged via the queue into bazelbuild:main with commit e42f8f4 Jun 6, 2024
4 checks passed
@aignas aignas deleted the fix/reproducible-lock-file branch June 6, 2024 08:16
@aignas
Copy link
Collaborator Author

aignas commented Jun 6, 2024

I don't have time today, and I would like to get #1837 into the release as well. If all goes well, the earliest I can do maybe tomorrow (or early next week) and Richard is on vacation this week. So early next week is probably a good worst case scenario.

@fmeum
Copy link
Contributor

fmeum commented Jun 6, 2024

No hurry, any time next week would be perfect!

github-merge-queue bot pushed a commit that referenced this pull request Jun 6, 2024
…epo (#1837)

With this change we add support for platform-specific wheel registration
and
doing the selection of which wheel is used at build time.

This supports:
- Different package versions for different platforms.
- Use string_flags to configure what to fetch/select:
    - only whls, only sdist or auto mode.
    - libc version and `musl` vs `glibc` selection.
    - universal2 vs arch wheels for mac.
    - target osx version selection.

Summary of changes:
- The `uv pip install` would only warn the user of yanked packages but
  would not refuse to install them. Update our implementation to better
  match the same behaviour.
- A previous PR has added the support for passing it in the
`requirements_by_platform` and this just add the necessary code to make
  sure that we can also do the dependency management when parsing the
  `whl` `METADATA` files.
- Only configure `dev_pip` deps for `linux` and `osx` platforms to not
raise
  issues later.
- Add a function for generating a `whl_library` name from a `filename`.
- Add a macro for generating all config settings for a particular set of
parameters.
- Update `render_pkg_aliases` to also use those config settings.
- Update the toolchain selection `target_settings` to use the
`py_linux_libc`
config setting. With this the user can register a `musl` linux toolchain
if
  needed. We can also use similar `flag_values` to resolve #1876.
- Integrate everything in the `pip` extension and setup cross-platform
  select statements.

Work towards #1357, #260

Stacked on #1937
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.

Make MODULE.bazel.lock file have the same contents no matter the platform bazel is executed on
3 participants