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(uv): fix UV_BIN usage with current_toolchain #2074

Merged
merged 3 commits into from
Jul 19, 2024

Conversation

aignas
Copy link
Collaborator

@aignas aignas commented Jul 18, 2024

Before this PR the uv toolchain could not be used in a genrule it
seems because the //python/uv:toolchain does not have the necessary
providers for using the make variables and the re-exporting of the make
variables from the toolchain in the current_toolchain rule did not
seem to work.

This PR removes the provider construction in the toolchain and does
that only in the current_toolchain. This better mirrors how the Python
toolchains are setup (grepping PYTHON3 is sufficient to get examples).
This also splits out work done in #2059 to decrease its scope, so that
this can be discussed separately.

Work towards #1975

Before this PR the `uv` toolchain could not be used in a `genrule` it
seems because the `//python/uv:toolchain` does not have the necessary
providers for using the make variables and the re-exporting of the make
variables from the `toolchain` in the `current_toolchain` rule did not
seem to work.

This PR removes the provider construction in the `toolchain` and does
that only in the `current_toolchain`. This better mirrors how the Python
toolchains are setup (grepping `PYTHON3` is sufficient to get examples).
This also splits out work done in bazelbuild#2059 to decrease its scope, so that
this can be discussed separately.

Work towards bazelbuild#1975
@aignas aignas requested a review from rickeylev as a code owner July 18, 2024 15:30
@@ -30,7 +30,9 @@ def _current_toolchain_impl(ctx):
# Bazel requires executable rules to create the executable themselves,
# so we create a symlink in this rule so that it appears this rule created its executable.
original_uv_executable = toolchain_info.uv_toolchain_info.uv[DefaultInfo].files_to_run.executable
symlink_uv_executable = ctx.actions.declare_file("uv_symlink_{}".format(original_uv_executable.basename))

# Use `uv` as the name of the binary to make the help message well formatted
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: The only reason I went with a weird name, was because I was worried about Windows. I might be wrong, but I thought Windows requires(ed) that executables have the .exe suffix?

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
Copy link
Collaborator Author

aignas commented Jul 19, 2024

Regarding the name of the binary, it seems that the CI is happy, so I'll roll
with it, having stable names can make debugging easier.

I think the .exe extension requirement is for output files from _binary rules but in this case it is not that, so let's see if it breaks anywhere in the future.

@aignas aignas enabled auto-merge July 19, 2024 01:26
@aignas aignas added this pull request to the merge queue Jul 19, 2024
Merged via the queue into bazelbuild:main with commit bd0ca99 Jul 19, 2024
4 checks passed
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.

2 participants