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(bzlmod): let workspace-invoked python_register_toolchains to register toolchains #2289

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

rickeylev
Copy link
Collaborator

@rickeylev rickeylev commented Oct 11, 2024

While migrating to bzlmod, users may have a hybrid build where WORKSPACE contains
python_register_toolchain() calls. Because these calls originate from WORKSPACE files,
the native.register_toolchain APIs are still available. At the same time, we still
detect that bzlmod is enabled, and thus disable calling those functions. The net
effect is, users aren't able to register toolchains in such a hybrid setup. At the
same time, because the code path is shared, we can't have the bzlmod toolchain code
try to call them, as it'll fail.

To accomodate both cases, have the bzlmod toolchain code pass a special arg so that
python_register_toolchains knows to skip parts that don't apply to the bzlmod toolchain
invocation.

This was all unwound by some users and pointed out in a Slack thread. A few people are
manually carrying an equivalent patch for a working hybrid mode.

Fixes #1675

@rickeylev rickeylev changed the title wip: bzlmod workspace interop fix(bzlmod): let workspace-invoked python_register_toolchains to register toolchains Oct 11, 2024
@rickeylev rickeylev force-pushed the fix.bzlmod.workspace.interop branch from d3ff7a6 to 0272f6c Compare October 11, 2024 02:44
@rickeylev rickeylev marked this pull request as ready for review October 11, 2024 02:45
@rickeylev rickeylev requested a review from aignas as a code owner October 11, 2024 02:45
@rickeylev rickeylev force-pushed the fix.bzlmod.workspace.interop branch from 0272f6c to 1a10cfd Compare October 11, 2024 02:48
Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

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

Nice!

@aignas aignas added this pull request to the merge queue Oct 11, 2024
Merged via the queue into bazelbuild:main with commit 43583d1 Oct 11, 2024
4 checks passed
@rickeylev rickeylev deleted the fix.bzlmod.workspace.interop branch October 29, 2024 03:40
mbland added a commit to mbland/rules_scala that referenced this pull request Nov 13, 2024
Resolves the duplicate repo instantiation errors from the previous merge
commit. They all had to do with repos that twitter_scrooge and scalafmt
have in commmon with scala_proto. Given that, passing the `scala_proto`
boolean parameter of `scala_toolchains` into the `twitter_scrooge` and
`scalafmt_repositories` functions seemed the easiest workaround for now.

I picked up the `**kwargs` + `kwargs.pop()` pattern up from:

- bazelbuild/rules_python#2289

It seems to be a pattern for passing through hidden, purely internal
parameters, not to be relied upon by any external users.
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.

Enabling bzlmod causes workspace toolchains to no longer be registered
2 participants