Handle VS Build Tools 2022 when trying to detect VS version.

bazelbuild / rules_cc

C++ Rules for Bazel

https://bazel.build

Apache License 2.0

176 stars 88 forks source link

Handle VS Build Tools 2022 when trying to detect VS version. #186

Open gabware opened 1 year ago

gabware commented 1 year ago

This only handles cases where the user explicitely set BAZEL_VC through the environment. Handling BAZEL_VS will require more changes I think. LMKWYT.

gabware commented 1 year ago

Added code to handle default locations for VS installation. Tried to minimize code duplication by generating the list of locations.

oquenchil commented 1 year ago

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

gabware commented 1 year ago

Sure. Couple questions before I do that:

We're talking about updating https://github.com/bazelbuild/bazel/blob/master/tools/cpp/windows_cc_configure.bzl . Correct?
How do I test that I didn't break anything ? What are the targets I should be building ?
I see there are quite a few differences between the two files. Is it ok if I redo only the changes from this PR? I think it'd be best to avoid potential build break and keep PR small and consistent.

oquenchil commented 1 year ago

We're talking about updating https://github.com/bazelbuild/bazel/blob/master/tools/cpp/windows_cc_configure.bzl . Correct?

Yes.

How do I test that I didn't break anything ? What are the targets I should be building ?

We don't test the toolchain files themselves. But if nothing breaks on CI then we're good.

I see there are quite a few differences between the two files. Is it ok if I redo only the changes from this PR? I think it'd be best to avoid potential build break and keep PR small and consistent.

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?

fmeum commented 1 year ago

CC @meteorcloudy, who is working on making rules_java the source of truth for Java toolchains (see https://github.com/bazelbuild/bazel/pull/18423).

Given that, I'm pretty sure the correct answer is rules_cc.

meteorcloudy commented 1 year ago

https://github.com/bazelbuild/bazel/pull/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 @bazel_tools.

@comius @ted-xie

meteorcloudy commented 1 year ago

I can take a look at using rules_cc as source of truth after finishing rules_java

gabware commented 1 year ago

Was it the version in bazel_tools or the one in rules_cc?

AFAICT the version in rules_cc seems cleaner; better maintained. Functions were moved to private, there are a couple fixes in, etc. I believe rules_cc should be the source of truth.

Do you think it makes sense Gabriel to update both here? Unless I'm mistaken, I'd have to do a different PR because https://github.com/bazelbuild/bazel is a different repo. So I won't "update both here" per-se. I'll update both as two PR. (unless there's a way to update both here that I don't know; I'm not super familiar with github yet).

meteorcloudy commented 1 year ago

@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)

oquenchil commented 1 year ago

Sounds like we can get this in then.

gabware commented 1 year ago

Anything left here or can we merge this ?