Skip to content

fix(uv): fix UV_BIN usage with current_toolchain#2074

Merged
aignas merged 3 commits intobazel-contrib:mainfrom
aignas:fix/uv-toolchain-make-var
Jul 19, 2024
Merged

fix(uv): fix UV_BIN usage with current_toolchain#2074
aignas merged 3 commits intobazel-contrib:mainfrom
aignas:fix/uv-toolchain-make-var

Conversation

@aignas
Copy link
Copy Markdown
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 bazel-contrib#2059 to decrease its scope, so that
this can be discussed separately.

Work towards bazel-contrib#1975
@aignas aignas requested a review from rickeylev as a code owner July 18, 2024 15:30
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 bazel-contrib:main with commit bd0ca99 Jul 19, 2024
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