-
-
Notifications
You must be signed in to change notification settings - Fork 51
feat(pip): Add a clean-sheet pip implementation #650
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
base: main
Are you sure you want to change the base?
Conversation
|
Still need to plumb in toolchains & real installer.
|
||
for minor in MINORS: | ||
selects.config_setting_group( | ||
name = "is_{}{}{}".format(interpreter, major, minor), |
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.
generally %s
is faster than .format
because it avoids a function call and has simpler (faster) arg conversion. Also if you have only a single substitution it helps even more because you don't even need to construct a tuple. Up to you where to use it, but at least for the functions in the extension that are called repeatedly I would strongly consider it. (Honestly I pretty much always use it unless I have a complex/multiline template with 4+ args and/or using an arg multiple times)
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.
since you said you love nits...i left a bunch of nits. feel free to apply or disregard as you see fit!
# We loop up to the second-to-last item to ensure we always have a 'next' stage. | ||
for i, current_stage in enumerate(stages): | ||
selects.config_setting_group( | ||
name = "{}".format(current_stage.name), |
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.
nit: name = current_stage.name
:)
selects.config_setting_group( | ||
name = "{}".format(current_stage.name), | ||
match_any = [ | ||
":{}".format(current_stage.condition), |
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.
":" + current_Stage.condition
or ":%s" % current_stage.condition
are both faster and less chars :)
""" | ||
selects.config_setting_group( | ||
name = "{}", | ||
match_all = {}, |
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.
it might be a bit clearer to put the brackets in the expression here, especially since I think the way you did it I think the trailing ]
isn't indented correctly? Here's a similar thing I did with build_deps
here
# Collect all hubs, ensure we have no dupes | ||
for mod in module_ctx.modules: | ||
for hub in mod.tags.declare_hub: | ||
hub_specs.setdefault(hub.hub_name, {}) |
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.
I saw you use setdefault
like this a bunch, I think you can tweak the pattern slightly to avoid repeated lookups:
hub_specs.setdefault(hub.hub_name, {})[mod.name] = 1
problems = [] | ||
for hub_name, modules in hub_specs.items(): | ||
if len(modules.keys()) > 1: | ||
problems.append( |
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.
I think you could combine this with the previous loop and make hub_specs
store hub_name -> mod_name(just check if you already have an entry and add to
problems) instead of of
hub_name -> mod_name -> 1`. I guess it would make it harder to handle 3 modules all using the same name, but meh? It would probably lead to simpler usage in the rest of the extension
load("//pip/private/constraints/platform:defs.bzl", "supported_platform") | ||
load(":parse_whl_name.bzl", "parse_whl_name") | ||
|
||
def format_arms(d): |
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.
https://bazelbuild.slack.com/archives/CDCMRLS23/p1757528874106669
do with this as you will :)
load(":parse_whl_name.bzl", "parse_whl_name") | ||
|
||
def format_arms(d): | ||
content = [" \"{}\": \"{}\"".format(k, v) for k, v in d.items()] |
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.
maybe clearer to use single quotes on the outside and/or repr
the k/v to avoid inner ones?
def sort_select_arms(arms): | ||
# {(python, platform, abi): target} | ||
pairs = list(arms.items()) | ||
pairs = sorted(pairs, key=select_key, reverse=True) |
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.
can you pass arms.items()
directly into sorted and skip the copy?
return [ | ||
# FIXME: Need to generate PyInfo here | ||
DefaultInfo( | ||
files = depset([ |
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.
i guess you could create this depset once and reuse it below
Design doc
An implementation of pip based on consuming PEP-751 [1] like lockfiles. Specifically uv lockfiles, which contain internal dependency graph information that the PEP-751 specification labels optional.
Follows in the footsteps of rules_js's pnpm support by consuming a lockfile which contains enough information to produce materialize dependencies without performing any repository time operations which could be platform dependent.
Features
Example
The active venv state can be overriden at the cli by specifying
--'@pip//venv=b'
here for instance, or by using transitions to(re) set that same flag.Appendix
[1] https://peps.python.org/pep-0751/
[2] https://peps.python.org/pep-0751/#locking-build-requirements-for-sdists
Changes are visible to end-users: yes
To do list
whl
files notpy_library
targets for pex etc.--editable
/ vendoring)rules_python
's config settings for nowrules_python
's config settings for now fix(toolchains): correctly register musl/freethreaded toolchains for workspace bazel-contrib/rules_python#3314--editable
-like workflowrules_python
's package name normalization so that migration is easier.rules_python
@hub//package[:package]
syntax?rules_rust
@hub//:packge
syntax?toml.bzl
workinguv
dependencyTest plan
py_venv_test
s covering that different versions of the same package can be concurrently configured via different venvspy_venv_test
covering that Airflow or another package with dependency cycles can be provisionedpy_venv_binary
embedded in and transitioned for a Linux OCI container across arch boundaries