feat: add support for passing extra pip args by platform #2745
Conversation
| @@ -4,8 +4,8 @@ | |||
| # (Note, we cannot use `common --deleted_packages` because the bazel version command doesn't support it) | |||
| # To update these lines, execute | |||
| # `bazel run @rules_bazel_integration_test//tools:update_deleted_packages` | |||
| build --deleted_packages=examples/build_file_generation,examples/build_file_generation/random_number_generator,examples/bzlmod,examples/bzlmod_build_file_generation,examples/bzlmod_build_file_generation/other_module/other_module/pkg,examples/bzlmod_build_file_generation/runfiles,examples/bzlmod/entry_points,examples/bzlmod/entry_points/tests,examples/bzlmod/libs/my_lib,examples/bzlmod/other_module,examples/bzlmod/other_module/other_module/pkg,examples/bzlmod/patches,examples/bzlmod/py_proto_library,examples/bzlmod/py_proto_library/example.com/another_proto,examples/bzlmod/py_proto_library/example.com/proto,examples/bzlmod/runfiles,examples/bzlmod/tests,examples/bzlmod/tests/other_module,examples/bzlmod/whl_mods,examples/multi_python_versions/libs/my_lib,examples/multi_python_versions/requirements,examples/multi_python_versions/tests,examples/pip_parse,examples/pip_parse_vendored,examples/pip_repository_annotations,examples/py_proto_library,examples/py_proto_library/example.com/another_proto,examples/py_proto_library/example.com/proto,gazelle,gazelle/manifest,gazelle/manifest/generate,gazelle/manifest/hasher,gazelle/manifest/test,gazelle/modules_mapping,gazelle/python,gazelle/pythonconfig,gazelle/python/private,tests/integration/compile_pip_requirements,tests/integration/compile_pip_requirements_test_from_external_repo,tests/integration/custom_commands,tests/integration/ignore_root_user_error,tests/integration/ignore_root_user_error/submodule,tests/integration/local_toolchains,tests/integration/pip_parse,tests/integration/pip_parse/empty,tests/integration/py_cc_toolchain_registered,tests/modules/other,tests/modules/other/nspkg_delta,tests/modules/other/nspkg_gamma | |||
There was a problem hiding this comment.
The pre-commit hook updated this 🤷
There was a problem hiding this comment.
I understand that the desire is to have per platform overrides, where we only care about the host platform.
If #2730 was not merged, the PR as is could not be merged because this creates different behaviour on different host platforms.
However, I would be much more keen to work with the target platform rather than the host platform. What is more, the direction where we are going with #260 does not align well with what is being introduced here if I understand correctly.
| `${VARNAME}` (expanding to empty string if unset) or | ||
| `${VARNAME:-default}` (expanding to default if the variable is unset | ||
| or empty in the environment), if `"VARNAME"` is listed in the | ||
| `envsubst` attribute. See also `envsubst`. |
There was a problem hiding this comment.
Please add a note when this has been added and we can link to the attributes in our docs.
| `envsubst` attribute. See also `envsubst`. | |
| {attr}`envsubst` attribute. See also {attr}`envsubst`. | |
| :::{versionadded} VERSION_NEXT_FEATURE | |
| ::: |
| "extra_pip_args_by_platform": attr.string_list_dict( | ||
| doc = """Similar to `extra_pip_args`. | ||
|
|
||
| Extra arguments to pass on to pip on the assoicated platform specifiers. Args must not contain spaces. |
There was a problem hiding this comment.
Could you please update https://github.com/bazel-contrib/rules_python/blob/main/docs/pypi-dependencies.md?plain=1#L320 section on how this feature could be used?
| os_name = "mac os x", | ||
| os_arch = "arch64", | ||
| read = lambda x: { | ||
| "darwin.txt": "simple==0.0.2 --hash=sha256:deadb00f", |
There was a problem hiding this comment.
Could you please add extra files so that we can see that there are differences on each platform?
| os = ctx.os.name.replace(" ", "") | ||
| arch = ctx.os.arch.replace(" ", "") |
There was a problem hiding this comment.
| os = ctx.os.name.replace(" ", "") | |
| arch = ctx.os.arch.replace(" ", "") | |
| os = ctx.os.name.replace(" ", "") | |
| arch = ctx.os.arch.replace(" ", "") |
This is duplicating code from repo_utils. We already have code for getting os and arch labels that the rest of the code understands. See host_platform function in parse_requirements.bzl (if I remember correctly).
| @@ -71,6 +72,8 @@ exports_files(["requirements.bzl"]) | |||
| """ | |||
|
|
|||
| def _pip_repository_impl(rctx): | |||
| extra_pip_args = resolve_extra_pip_args(rctx, rctx.attr.extra_pip_args, rctx.attr.extra_pip_args_by_platform) | |||
There was a problem hiding this comment.
Do I understand correctly that this will resolve the pip args depending on the host platform? What about resolving based on the target platform? I'd rather pass the rctx.attr.extra_pip_args and rctx.attr.extra_pip_args_by_platform to the requirements_files_by_platform and resolve the pip args by target platform.
The current API is somewhat not great at doing the overrides per platform because we need to have these dictionaries for the overrides and if we add this, then we'll have to maintain this until v2.
There was a problem hiding this comment.
Can you get the target platform in this context?
The way requirements_files_by_platform seems to work is to inject the flags into the requirement lines, and pass the platform flag to pip, does that work with other flags too?
The use case we have here is that we need to force binary only fetching on certain platforms, where as on others we can't do that and allow the sdist to be built. There are some other flags we pass only on Linux too, but I don't recall them, can check tomorrow.
There was a problem hiding this comment.
I created #2747 to create a better API for this, but in the mean time this is how I can see things fit:
requirements_by_platformparses the files and populates the initial data structure with theextra_pip_argsthat are common for all platforms. By this time we have a list ofrequirementsstructs that have: the requirement line that is needed, extra_pip_args for that requirement line and the target platform.- (this is where this PR could plug in) - for each target platform listed by the user we add the extra_pip_args based on the already defined target platform.
- If we see that there is a single
requirementstruct for target platformsfooandbarand we need to add 2 different sets of pip args, then we split the struct into two - one forfoo, one forbarand we add the pip args.
- then later in the code there is code that actually creates the
whl_libraryinstances and wires everything together through the hub repo.
Supports passing extra arguments to pip based on the host platform. This is useful when arguments such as
--only-binaryare platform dependant.