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.

Sign up for GitHub

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

Jump to bottom

feat(toolchain, pip.parse): introduce a new '_host' toolchain repo #1644

Merged
merged 1 commit into from
Jan 9, 2024
Merged

feat(toolchain, pip.parse): introduce a new '_host' toolchain repo #1644

merged 1 commit into from
Jan 9, 2024

Conversation

aignas
Copy link
Collaborator

@aignas aignas commented Dec 21, 2023 •
edited
Loading

This is for passing it in repository_rules and relies on the canonical
label representation introduced in bazel 6.0 and symlink support (needs
to be present on Windows) to work.

This allows the users to not need to load the interpreter label from
a .bzl file but instead specify the label in the form of
@<mytoolchainrepo>_host//:python.

In order to make it work robustly on Windows, we do
repository_ctx.path(python_interpreter_label).realpath to get the actual
path of the Python interpreter on both, bzlmod and non-bzlmod setups on
Windows within the whl_library repository rule.

Work towards #1643.

@aignas aignas marked this pull request as ready for review December 21, 2023 01:35
@rickeylev
Copy link
Collaborator

Oh this is gonna be tough to remember. @chrislovecnm do you remember what the issue was with windows and trying to symlink to the python interpreter from another repo?

@rickeylev
Copy link
Collaborator

Ah, this might be it: #1265

Something about: the symlink can't be executed directly / powershell issue / dll's can't be found from the symlink's location ?

@aignas aignas force-pushed the feat/setup-a-host-interpreter-for-external-repo-usage branch from 5f65e16 to 4840d42 Compare December 21, 2023 08:22
@aignas
Copy link
Collaborator Author

aignas commented Dec 21, 2023

I am wondering then why the CI is still passing. Maybe the
repository_ctx usage is good enough with symlinks, because
the symlink gets always resolve the real path before executing?

Just to be explicit, I am not using the symlink in toolchain
resolution. For that we already have the alias repos and this
is only for the repository_ctx usecase and the Windows CI seems
to be still happy. Do we need to make this host_python repo
internal to rules_python in some way?

@aignas
Copy link
Collaborator Author

aignas commented Dec 21, 2023

Yeah, the previous path.realpath usage was only on Windows bzlmod, when I enabled it for everything, it started working again (the multi-python-version tests on non-bzlmod started failing because the path.realpath workaround was missing there.

@aignas aignas force-pushed the feat/setup-a-host-interpreter-for-external-repo-usage branch from 1a97810 to 7cf8787 Compare December 21, 2023 23:28
rickeylev
rickeylev reviewed Dec 22, 2023
View reviewed changes
examples/multi_python_versions/WORKSPACE Show resolved Hide resolved
examples/pip_parse_vendored/BUILD.bazel Show resolved Hide resolved
examples/pip_parse_vendored/BUILD.bazel Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@aignas aignas force-pushed the feat/setup-a-host-interpreter-for-external-repo-usage branch from 7cf8787 to 53a553e Compare December 22, 2023 13:06
@aignas
feat(toolchain, pip.parse): introduce a new '_host' toolchain repo
991d4c0 
This is for passing it in repository_rules and relies on the canonical
label representation introduced in bazel 6.0 and symlink support (needs
to be present on Windows) to work.

This allows the users to not need to `load` the interpreter label from
a `.bzl` file but instead specify the label in the form of
`@<mytoolchainrepo>_host//:python`.

Work towards bazelbuild#1643.
@aignas aignas force-pushed the feat/setup-a-host-interpreter-for-external-repo-usage branch from 53a553e to 991d4c0 Compare January 9, 2024 00:32
@rickeylev rickeylev added this pull request to the merge queue Jan 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 9, 2024
@aignas aignas added this pull request to the merge queue Jan 9, 2024
Merged via the queue into bazelbuild:main with commit 4fe0db3 Jan 9, 2024
4 checks passed
@aignas aignas mentioned this pull request Jan 10, 2024
Closed
4 tasks
@aignas aignas deleted the feat/setup-a-host-interpreter-for-external-repo-usage branch May 13, 2024 06:49
mikex-oss added a commit to mikex-oss/bazel_rules_hdl that referenced this pull request Jun 26, 2024
@mikex-oss
Fixes to allow downstream project to set a different Python toolchain.
1523bb3 
Previously, in the typical use case where `python_interpreter_target`
is passed, the @bazel_rules_hdl//:init.bzl init macro would always
call the vendored `install_deps`. However, this would only work if
the passed in toolchain name matched the one used to generate the
vendored requirements.bzl (since @python39 is hardcoded as a default
inside).

Now, we pass along any interpreter args to the vendored `install_deps`
to override the defaults as documented at:
https://github.com/bazelbuild/rules_python/blob/main/examples/pip_parse_vendored/README.md.
This also eliminates the documented requirement to use the generated
pip deps instead of the vendored version if an interpreter arg is
set.

We also update rules_python to the latest version, allowing the use
of the "_host" toolchain label to avoid loading the interpreter per
bazelbuild/rules_python#1644.

Finally, we clean up other references to the pip_parse repo (which
was only intended for the vendoring) and move them to use the
vendored deps directly.
mikex-oss added a commit to mikex-oss/bazel_rules_hdl that referenced this pull request Jun 26, 2024
@mikex-oss
Fixes to allow downstream project to set a different Python toolchain.
ef258d9 
Previously, in the typical use case where `python_interpreter_target`
is passed, the @bazel_rules_hdl//:init.bzl init macro would always
call the vendored `install_deps`. However, this would only work if
the passed in toolchain name matched the one used to generate the
vendored requirements.bzl (since @python39 is hardcoded as a default
inside).

Now, we pass along any interpreter args to the vendored `install_deps`
to override the defaults as documented at:
https://github.com/bazelbuild/rules_python/blob/main/examples/pip_parse_vendored/README.md.
This also eliminates the documented requirement to use the generated
pip deps instead of the vendored version if an interpreter arg is
set.

We also update rules_python to the latest version, allowing the use
of the "_host" toolchain label to avoid loading the interpreter per
bazelbuild/rules_python#1644.

Finally, we clean up other references to the pip_parse repo (which
was only intended for the vendoring) and move them to use the
vendored deps directly.
mikex-oss added a commit to mikex-oss/bazel_rules_hdl that referenced this pull request Jun 26, 2024
@mikex-oss
Fixes to allow downstream project to set a different Python toolchain.
949a0de 
Previously, in the typical use case where `python_interpreter_target`
is passed, the @bazel_rules_hdl//:init.bzl init macro would always
call the vendored `install_deps`. However, this would only work if
the passed in toolchain name matched the one used to generate the
vendored requirements.bzl (since @python39 is hardcoded as a default
inside).

Now, we pass along any interpreter args to the vendored `install_deps`
to override the defaults as documented at:
https://github.com/bazelbuild/rules_python/blob/main/examples/pip_parse_vendored/README.md.
This also eliminates the documented requirement to use the generated
pip deps instead of the vendored version if an interpreter arg is
set.

We also update rules_python to the latest version, allowing the use
of the "_host" toolchain label to avoid loading the interpreter per
bazelbuild/rules_python#1644.

Finally, we clean up other references to the pip_parse repo (which
was only intended for the vendoring) and move them to use the
vendored deps directly.
mikex-oss added a commit to mikex-oss/bazel_rules_hdl that referenced this pull request Jun 26, 2024
@mikex-oss
Fixes to allow downstream project to set a different Python toolchain.
8044239 
Previously, in the typical use case where `python_interpreter_target`
is passed, the @bazel_rules_hdl//:init.bzl init macro would always
call the vendored `install_deps`. However, this would only work if
the passed in toolchain name matched the one used to generate the
vendored requirements.bzl (since @python39 is hardcoded as a default
inside).

Now, we pass along any interpreter args to the vendored `install_deps`
to override the defaults as documented at:
https://github.com/bazelbuild/rules_python/blob/main/examples/pip_parse_vendored/README.md.
This also eliminates the documented requirement to use the generated
pip deps instead of the vendored version if an interpreter arg is
set.

We also update rules_python to the latest version, allowing the use
of the "_host" toolchain label to avoid loading the interpreter per
bazelbuild/rules_python#1644.

Finally, we clean up other references to the pip_parse repo (which
was only intended for the vendoring) and move them to use the
vendored deps directly.
mikex-oss added a commit to mikex-oss/bazel_rules_hdl that referenced this pull request Jun 26, 2024
@mikex-oss
Fixes to allow downstream project to set a different Python toolchain.
abd2335 
Previously, in the typical use case where `python_interpreter_target`
is passed, the @bazel_rules_hdl//:init.bzl init macro would always
call the vendored `install_deps`. However, this would only work if
the passed in toolchain name matched the one used to generate the
vendored requirements.bzl (since @python39 is hardcoded as a default
inside).

Now, we pass along any interpreter args to the vendored `install_deps`
to override the defaults as documented at:
https://github.com/bazelbuild/rules_python/blob/main/examples/pip_parse_vendored/README.md.
This also eliminates the documented requirement to use the generated
pip deps instead of the vendored version if an interpreter arg is
set.

We also update rules_python to the latest version, allowing the use
of the "_host" toolchain label to avoid loading the interpreter per
bazelbuild/rules_python#1644.

Finally, we clean up other references to the pip_parse repo (which
was only intended for the vendoring) and move them to use the
vendored deps directly.
mikex-oss added a commit to mikex-oss/bazel_rules_hdl that referenced this pull request Jun 26, 2024
@mikex-oss
Fixes to allow downstream project to set a different Python toolchain.
7a1ba0e 
Previously, in the typical use case where `python_interpreter_target`
is passed, the @bazel_rules_hdl//:init.bzl init macro would always
call the vendored `install_deps`. However, this would only work if
the passed in toolchain name matched the one used to generate the
vendored requirements.bzl (since @python39 is hardcoded as a default
inside).

Now, we pass along any interpreter args to the vendored `install_deps`
to override the defaults as documented at:
https://github.com/bazelbuild/rules_python/blob/main/examples/pip_parse_vendored/README.md.
This also eliminates the documented requirement to use the generated
pip deps instead of the vendored version if an interpreter arg is
set.

We also update rules_python to the latest version, allowing the use
of the "_host" toolchain label to avoid loading the interpreter per
bazelbuild/rules_python#1644.

Finally, we clean up other references to the pip_parse repo (which
was only intended for the vendoring) and move them to use the
vendored deps directly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rickeylev rickeylev rickeylev approved these changes

@f0rmiga f0rmiga Awaiting requested review from f0rmiga

@chrislovecnm chrislovecnm Awaiting requested review from chrislovecnm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aignas @rickeylev