-
Notifications
You must be signed in to change notification settings - Fork 133
Add Bazel 8 compatibility with automatic bzlmod/WORKSPACE detection #752
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
base: main
Are you sure you want to change the base?
Conversation
|
Another avenue to look into is this and the code that generates WORKSPACE files. I think the latest version of bazel doesn't support WORKSPACE files at all and that may need to be updated to be a really bare bones module file file |
eb7833c to
afe8be5
Compare
|
@chickenandpork you mentioned me in #669. i wanted to ask you if you have some resources to help / experience with bazel builds on windows? |
I don't have Windows; haven't used it with any efficiency since 2004. I've simply got a lot of different MacOS. I'd likely blindly go at each item such as: |
c412a1a to
e19343c
Compare
117c03e to
5265a11
Compare
|
@achew22, I'd be happy to get a review now that CI is green. I spent some time debugging the Windows CI failures and found that migrating to the external rules_shell dependency (required in Bazel 8.0) fixed the issues. (i ended up running an unsigned version of windows 10 in a vm on my linux machine, that helped a bit but it sure wasnt pretty) |
|
Stale pull request message |
|
@janwinkler1 is this still necessary? I was, maybe wrongly, under the impression that bzlmod was now supported. Can you correct my understanding? |
|
@achew22 ty for your comment, i will look into it. |
| "-k", | ||
| "--nocache_test_results", | ||
| "--noenable_bzlmod", | ||
| "--noenable_workspace", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @janwinkler1 with changes like this, and where bazel changes an --experimental_<something> into a --<something> do we need to start determining the bazel version before choosing which arguments pass-through ? Or should we keep a super-set of all arguments, and let bazel figure out which don't work for its version ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chickenandpork TBH i am a bit torn on what the best approach might be. AFAIU
bazel 7: WORKSPACE is the default, BZLMOD needs to be enabled
bazel 8: WORKSPACE needs to be enabled, BZLMOD is the default
bazel 9: according to https://bazel.build/about/roadmap WORKSPACE might not even be supported anymore, BZLMOD is the default
therefore, if we want to cater all versions, something where bazel "figures this out itself" might be the best option? altough not the safest?
but ATM i was too invested with trying to fight windows, without a windows PC :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janwinkler1 your efforts are appreciated too -- I don't think any of us has a Windows box online :)
The CI presents its own challenges.
Are you on the ibazel slack channel ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chickenandpork thank you! no i am not :)
9f160db to
0fd4e4f
Compare
@achew22 AFAIU yes it would still be necessary, though i have very limited time ATM to work on this. it is directly related to the bazel 8. on ubuntu and macos we dont need the shell toolchains. on windows however we need them. therefore for our test harness we need to inject them into every ephemeral workspace. does this answer your question? i wasnt entirely sure what you were referring to with "this" |
|
Stale pull request message |
|
My general rule of thumb is still in force here. Happy to take a look once we get CI green |
symlink_dir_test: IIUC this tests file watching through symlinks but Windows symlinks require admin privileges and behave fundamentally differentlyincompatible_by_default_test: IIUC this tests platform constraint system with shell scripts; here, Windows users would typically use batch files/executables, not shell scripts for platform-specific binaries