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

Make installer work with bazel 7 #2089

Closed
wants to merge 1 commit into from
Closed

Make installer work with bazel 7 #2089

wants to merge 1 commit into from

Conversation

hzeller
Copy link
Collaborator

@hzeller hzeller commented Jan 31, 2024

This is work in progress, playing with what needs to be done.

One of the issues to get things to work with bazel7 is that the installer does not work, it creates this error message:

verible/BUILD:49:10: in (an implicit dependency) attribute of _gen_installer rule //:_install_gen: in $whitelist_function_transition attribute of _gen_installer rule //:_install_gen: package group '@@bazel_tools//tools/whitelists/function_transition_whitelist:function_transition_whitelist' is misplaced here (they are only allowed in the visibility attribute).

I hacked around here by adding a patch that removes that line :)

Things to work on

  • Make the choice if the patch is applied depending on the bazel version either in WORKSPACE or maybe in the installer file itself. I have no experience with Skylark (or, Python for that matter) to know what to do. Hopefully reviewers on this PR can help.
  • This is a hack of course, but once we know what needs to be done, we could upstream it. The upstream project is kind-of dormant, but it might be possible to get things in (I might have commit permission)
  • Also, given the unmaintained state of the installler, we could consider not using the installer and providing that manually.

There are other issues with bazel 7: for some reason the protocol buffers are completely borked, but that is for another day :)

@hzeller hzeller requested a review from corco January 31, 2024 05:01
@hzeller
Copy link
Collaborator Author

hzeller commented Jan 31, 2024

Jonathan, you're the only one close to this project who I imagine knows bazel enough to help figuring these things out.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (31182be) 92.96% compared to head (7073dc2) 92.92%.
Report is 4 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2089      +/-   ##
==========================================
- Coverage   92.96%   92.92%   -0.04%     
==========================================
  Files         358      358              
  Lines       26563    26582      +19     
==========================================
+ Hits        24693    24701       +8     
- Misses       1870     1881      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@corco
Copy link
Collaborator

corco commented Jan 31, 2024

Supporting bazel 5 through 7 looks complicated. Since we do not use bzlmod, we need to add --noenable-bzlmod to the bazel build command. Normally, this would be in .bazelrc, but bazel 5 will fail since that option did not exist then. I didn't see any way to conditionally add options in bazelrc, but it could be documented to use that flag on bazel 7.

Your patch has a similar problem, it is required for bazel 7, but bazel 5/6 fails if it is applied. I don't know if it's possible (or more accurately, how hard it is) to conditionally apply a patch based on bazel version.

Looking at tensorflow and kythe, both use .bazelversion. I honestly don't see much worth in trying to support multiple bazel versions compared to asking developers to use bazelisk

@hzeller
Copy link
Collaborator Author

hzeller commented Jan 31, 2024

Yeah, kythe was the first project that I encountered that used .bazelversion, and it created major headaches back then and now.

So I think for now, we just have to tell developers to use bazel 5 or 6 and not 7, but do not write any .bazelversion as it is fundamentally broken the way it is implemented in bazel and bazelisk.

@hzeller hzeller closed this Feb 2, 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.

3 participants