-
Notifications
You must be signed in to change notification settings - Fork 98
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
Handle VS Build Tools 2022 when trying to detect VS version. #186
base: main
Are you sure you want to change the base?
Conversation
Added code to handle default locations for VS installation. |
Could you please send a PR for this to the copy of the file in bazelbuild/bazel's bazel_tools so that they are less out of sync? Thanks |
Sure. Couple questions before I do that:
|
Yes.
We don't test the toolchain files themselves. But if nothing breaks on CI then we're good.
Absolutely, only the changes from this PR. But I'm even starting to hesitate whether you should spend time on that. @fmeum What did we decide a few months back that should eventually be source of truth? Was it the version in bazel_tools or the one in rules_cc? Do you think it makes sense Gabriel to update both here? |
CC @meteorcloudy, who is working on making Given that, I'm pretty sure the correct answer is |
bazelbuild/bazel#18423 is almost done, and looks like a success. I think we should probably do the same for rules_cc, rules_android to remove more stuff from |
I can take a look at using rules_cc as source of truth after finishing rules_java |
AFAICT the version in
|
@gabware Yes, I think Pedro meant we should also update the toolchain configuration in the Bazel repo to keep this in sync until we actually switch to rules_cc as the source of truth (meaning loading toolchains from rules_cc by default in Bazel) |
Sounds like we can get this in then. |
Anything left here or can we merge this ? |
This only handles cases where the user explicitely set BAZEL_VC through the environment.
Handling BAZEL_VS will require more changes I think.
LMKWYT.