- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 189
windows support #827
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?
windows support #827
Conversation
| edit: no .bazelrc.user required | 
| 
 | 
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.
This looks great, i am okay with landing this as is, if you could selectively disable tests that doesn't work windows.
| @thesayyn I now have bsdtar fixed on Windows; thanks to @alexeagle for pushing the patch through all the repos. rules_oci now looks like it is working pretty well. 
 Thanks! | 
| @thesayyn ping | 
| @thesayyn This is ready to go - except the docs update which I can't run from windows. Please would you do this and advise on next steps? Thank you! cc @alexeagle @fmeum | 
| @thesayyn I fixed the bazel-lib docs macros whilst I was waiting :-) so these docs files are now updated and this is ready to go. | 
83252d5    to
    2d955fd      
    Compare
  
    | @fmeum I made some fixes; could you please trigger CI again? | 
| @thesayyn any chance you can take another look at this PR? :) | 
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.
This is looking closer. There seems to be some bad diff from formatters and feature additions which i'd like to address in a separate PR.
4186047    to
    041ff88      
    Compare
  
    041ff88    to
    5d2ae2e      
    Compare
  
    | @thesayyn Thanks for your review; I've addressed all your comments; moved unrelated features out; rebased to latest | 
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.
Looking great, a few small changes.
| I'll merge this once the windows cfg selection is fixed. | 
| @peakschris it would be nice to get windows CI back. do you mind adding that to the matrix? Also bcr presubmit will need to change to test for windows there. | 
| 
 I've added to bcr presubmit; happy to make a change to github workflows if you can guide me - I'm not familiar with this and superficially it looks like windows is already configured | 
| @thesayyn trying to get windows CI added & not hanging by following pattern from bazel-lib; can you give me rights to trigger/retrigger the workflow? | 
| @thesayyn I could use some advice. Building case4_transition with Bazel 8.3.1 or 8.4.1: results in ctx.toolchains["@bazel_tools//tools/sh:toolchain_type"].path returning /bin/bash instead of C:\msys64\usr\bin\bash.exe as set in BAZEL_SH envvar. This results in the bat wrappers generated for windows failing with invalid path. Is there any way to get the @bazel_tools//tools/sh:toolchain_type without it being interfered by the transition? This is only an issue with bazel 8; bazel 7.6.1 returns the windows bash.exe Thanks! Chris | 
| @thesayyn @mostynb I'm going to give up on this for now; I'm unable to get all the tests green for the same reasons that this PR struggled: #842 Upgrading to a version of bazel_lib that contains a tar that works on Windows requires us to drop support for modules under bazel 6; as @mostynb argues we could consider dropping bazel 6 completely. If you can get #842 merged then I can have another go with this. | 
cc97f2c    to
    f609908      
    Compare
  
    f609908    to
    87d6e02      
    Compare
  
    | Thanks, I'll see how i can help. | 
|  | ||
| # Create the host platform repository transitively required by bazel-lib | ||
|  | ||
| maybe( | 
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.
Hmm, why this is needed? i thought we simply dropped this requirement with some release of bazel_lib.
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.
I'm not sure if it was needed; I was seeing strange errors relate to resolving zstd toolchain so I pasted it in from bazel_lib release notes.
| 
 Thank you! | 
| Here's an attempt to upgrade CI to test bazel 7 and 8: #854 | 

Most features are working
Adds:
Limitations
Fixes:
_maybe_wrap_launcher_for_windowsselects wrapper based on target platform #420@@bazel_tools//src/conditions:host_windows_x64_constraintbeing used byoci_image#819Other relevant PRs
Test results:
bazel test //... --action_env=PATH - 36 pass and 14 skipped
bazel test //... --action_env=PATH --enable_runfiles - 36 pass and 14 skipped