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

Migration needed for Automatic Exec Groups #451

Open
sgowroji opened this issue Oct 9, 2024 · 4 comments
Open

Migration needed for Automatic Exec Groups #451

sgowroji opened this issue Oct 9, 2024 · 4 comments

Comments

@sgowroji
Copy link

sgowroji commented Oct 9, 2024

Migration to Automatic Exec Groups (AEGs) is needed since its incompatible flag (--incompatible_auto_exec_groups) will be enabled for Bazel@Head by default next week (week of 14th of October), thus it will be breaking core. Please migrate and fix potential errors, in order to unblock your project and users who depend on your project. Potential errors can be triggered by building your project with this incompatible flag. Note that AEGs will not be flipped by default for Bazel 8, but for Bazel at head after Bazel 8 cut.

For more details, please check:

Never heard of incompatible flags before? We have documentation that explains everything.

If you have any questions, please file an issue in https://github.com/bazelbuild/bazel/issues.

@ensonic
Copy link
Contributor

ensonic commented Oct 9, 2024

> git grep ctx.actions.run
bazel/app_chart.bzl:    ctx.actions.run_shell(
bazel/build_rules/helm_chart.bzl:    ctx.actions.run_shell(

@sgowroji what did you do to determine that we need a migration? I've build with --incompatible_auto_exec_groups and I think contrary to the docs, it also applies to run_shell) - I guess I just set toolchain to None, right

ensonic added a commit that referenced this issue Oct 9, 2024
This prepares us for bazel-head. We still need to bump used rules (e.g. rules_oci) when they ship a fix.

See #451.
@ensonic
Copy link
Contributor

ensonic commented Oct 9, 2024

@sgowroji as much as we'd like to fix it, I believe the common bazel rules need to make the first move.

ensonic added a commit that referenced this issue Oct 10, 2024
This prepares us for bazel-head. We still need to bump used rules (e.g.
rules_oci) when they ship a fix.

See #451.
@kotlaja
Copy link

kotlaja commented Oct 15, 2024

Hi @ensonic, Bazel team member here. Yes, this applies also to run_shell, which is documented in the above mentioned Bazel documentation (see The same is effective with ctx.actions.run_shell where toolchain parameter should be added when tools are from a toolchain).

And yes, sometimes toolchain needs to be set to None since it's not possible to always determine if the executable is coming from a toolchain or not (documented in the first potential error message in the migration section).

as much as we'd like to fix it, I believe the common bazel rules need to make the first move.

I see a commit after this - Is the migration done or do you still have some questions?

@ensonic
Copy link
Contributor

ensonic commented Oct 16, 2024

I see a commit after this - Is the migration done or do you still have some questions?

I've changed it for this repo but it is hard to say when it is done for all deps. I've also just sent a PR to bump rules_oci, lets see.
But no more questions, thanks.

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

No branches or pull requests

3 participants