rules_cc icon indicating copy to clipboard operation
rules_cc copied to clipboard

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

Open gabware opened this issue 2 years ago • 11 comments

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 avatar May 10 '23 07:05 gabware

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

gabware avatar May 11 '23 05:05 gabware

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

oquenchil avatar May 26 '23 12:05 oquenchil

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.

gabware avatar May 26 '23 16:05 gabware

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?

oquenchil avatar May 30 '23 11:05 oquenchil

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.

fmeum avatar May 30 '23 12:05 fmeum

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 avatar May 30 '23 13:05 meteorcloudy

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

meteorcloudy avatar May 30 '23 13:05 meteorcloudy

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

gabware avatar May 30 '23 17:05 gabware

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

meteorcloudy avatar May 31 '23 07:05 meteorcloudy

Sounds like we can get this in then.

oquenchil avatar Jun 06 '23 08:06 oquenchil

Anything left here or can we merge this ?

gabware avatar Jun 19 '23 19:06 gabware