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

Add toolchain param to affected actions #668

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kotlaja
Copy link

@kotlaja kotlaja commented Nov 21, 2023

This is part of migrations of Automatic Exec Groups (AEGs) where a toolchain parameter should be set inside actions which use executable or tools from a toolchain.

AEGs are under an incompatible flag right now, but will be flipped soon in Bazel 7. I've added this change since I was manually fixing rules_nodejs, which depends on bazel-lib. I can't fully test this change since I need at least Bazel 7, but Bazel@Head raises non related errors to my change. That's why there might be more related updates once bazel-lib supports Bazel 7.

Still, as I already mentioned, adding a toolchain parameter is masked by the incompatible flag, so these changes don't do anything right now. But it can help you with your migration in the near future when AEGs are enabled by default and you are forced to update actions which use toolchains.

Also note that the toolchain parameter is available from Bazel 6, but the full logic (and the incompatible flag) will be available from Bazel 7.

Please check official documentation for Automatic Exec Groups for additional info.


Type of change

  • New feature or functionality (change which adds functionality)

Test plan

  • Manual testing; please provide instructions so we can reproduce:
  • Use (at least) Bazel 7 and add --incompatible_auto_exec_groups flag.

@CLAassistant
Copy link

CLAassistant commented Nov 21, 2023

CLA assistant check
All committers have signed the CLA.

@kotlaja
Copy link
Author

kotlaja commented Nov 24, 2023

Hi @alexeagle, Nevena here from Bazel team in Munich! :) Can you please take a look at this PR?

@kormide
Copy link
Member

kormide commented Nov 24, 2023

Looks like this needs a bazel run //docs:update for docs tests.

@kormide
Copy link
Member

kormide commented Nov 24, 2023

@alexeagle Do you think we want this backported to the 1.x branch as well?

@alexeagle
Copy link
Member

We can say that bazel-lib 1.x doesn't support Bazel 7.0 so I don't think we need to backport this. If someone complains I can revisit.

@alexeagle
Copy link
Member

@kotlaja I tried to update the docs for you, but you didn't check the "allow maintainers to add commits" box

% git commit -a -m "bazel run docs:update"
[kotlaja/main f452708] bazel run docs:update
 1 file changed, 2 insertions(+), 1 deletion(-)
alexeagle@MacBook-Pro-8 bazel-lib % git push
Enumerating objects: 7, done.
Counting objects: 100% (7/7), done.
Delta compression using up to 8 threads
Compressing objects: 100% (4/4), done.
Writing objects: 100% (4/4), 493 bytes | 44.00 KiB/s, done.
Total 4 (delta 3), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (3/3), completed with 3 local objects.
To github.com:kotlaja/bazel-lib.git
 ! [remote rejected] kotlaja/main -> kotlaja/main (permission denied)
error: failed to push some refs to 'github.com:kotlaja/bazel-lib.git'

@kotlaja
Copy link
Author

kotlaja commented Nov 29, 2023

Sorry, I was on a vacation. Thanks for the help here!

Hm, allow maintainers to add commits was already checked, not sure why it didn't work.
I've run the command now, please take another look.

@thesayyn
Copy link
Member

thesayyn commented Dec 8, 2023

@kotlaja i meant to speak with somebody from bazel team about this. does adding this parameter make the action incompatible with older versions of bazel where this attribute isn't recognized or does it get simply ignored?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

5 participants