replace accesses to cc_toolchain.compiler_executable with calls to cc_toolchain.get_tool_for_action() #274

hypdeb · 2024-09-08T19:26:29Z

According to the Bazel documentation, compiler_executable is going to be phased out. It is already an issue to use it for people who are trying to work with the new rules_cc implementation of the cc rules.

…_toolchain.get_tool_for_action()

hypdeb · 2024-09-08T19:30:50Z

~~Would want to mark as draft until I've tested it, but not sure how 🤔 .~~

Found it.

hypdeb · 2024-09-09T10:50:44Z

Not as easy as I thought. The current error message suggests that find_cpp_toolchain is returning the cuda toolchain instead of the resolved cpp one. Absolutely no idea how this could happen.

hypdeb · 2024-09-09T14:22:26Z

That seems to do it, but I'm not sure exactly what these fragments are. Advice would be greatly appreciated.

hypdeb · 2024-09-09T20:04:33Z

A quick question: are all the include directories of the host compiler supposed to be passed to nvcc? It seems it's not the case with my custom cc_toolchain. It's probably unrelated to this MR though.

cuda/private/toolchain.bzl

docs/developer_docs.bzl

cloudhan · 2024-09-10T12:09:34Z

Otherwise, looks good to me.

NOTE: If the env_entry supports iteration like flag_group, we can change it to use the configured cc_toolchain from the action and pass it via the compile/link variable.

cloudhan · 2024-09-10T12:12:42Z

are all the include directories of the host compiler supposed to be passed to nvcc

@hypdeb Yes. The nvcc is just a wrapper in the host compiling case. So all the envs (automatically transitive if you don't specify some weird env flag) and flags (-Xcompiler) should be passed to host compiler to make it work as expected.

hypdeb · 2024-09-10T12:53:48Z

@hypdeb Yes. The nvcc is just a wrapper in the host compiling case. So all the envs (automatically transitive if you don't specify some weird env flag) and flags (-Xcompiler) should be passed to host compiler to make it work as expected.

Alright then there's probably some incompatibility between how rules_cc creates compiler flags and how they are imported here that causes my issue. I'll dig deeper into it. It's not directly related to that PR.

cloudhan · 2024-09-10T15:12:48Z

/test

cloudhan · 2024-09-10T17:05:18Z

Please rebase. I might trigger the integration test after merge. Seems some actions update contains breaking change for /test comment trigger

cloudhan · 2024-09-11T10:49:50Z

cuda/private/rules/cuda_objects.bzl

@@ -1,9 +1,9 @@
-load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain")
+load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain", "use_cpp_toolchain")


Yeah I just now realized what you meant with your first review comment. Just to confirm: we need to use the use_cpp_toolchain defined in rules_cuda because we support versions of Bazel where it is not defined in bazel_tools correct?

There's no way I suppose to define this one conditionally on the Bazel version? Not that I necessarily think it would be better, mostly out of curiosity.

cloudhan · 2024-09-11T10:50:01Z

cuda/private/rules/cuda_objects.bzl

 load("//cuda/private:actions/compile.bzl", "compile")
 load("//cuda/private:cuda_helper.bzl", "cuda_helper")
 load("//cuda/private:providers.bzl", "CudaInfo")
 load("//cuda/private:rules/common.bzl", "ALLOW_CUDA_HDRS", "ALLOW_CUDA_SRCS")
-load("//cuda/private:toolchain.bzl", "find_cuda_toolchain", "use_cpp_toolchain", "use_cuda_toolchain")
+load("//cuda/private:toolchain.bzl", "find_cuda_toolchain", "use_cuda_toolchain")


cloudhan · 2024-09-11T10:50:11Z

cuda/private/rules/cuda_library.bzl

 load("//cuda/private:actions/compile.bzl", "compile")
 load("//cuda/private:actions/dlink.bzl", "device_link")
 load("//cuda/private:cuda_helper.bzl", "cuda_helper")
 load("//cuda/private:providers.bzl", "CudaInfo")
 load("//cuda/private:rules/common.bzl", "ALLOW_CUDA_HDRS", "ALLOW_CUDA_SRCS")
-load("//cuda/private:toolchain.bzl", "find_cuda_toolchain", "use_cpp_toolchain", "use_cuda_toolchain")
+load("//cuda/private:toolchain.bzl", "find_cuda_toolchain", "use_cuda_toolchain")


cloudhan · 2024-09-11T10:50:20Z

cuda/private/rules/cuda_library.bzl

@@ -1,10 +1,10 @@
-load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain")
+load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain", "use_cpp_toolchain")


I think you forgot to revert these changes

cloudhan · 2024-09-11T10:50:55Z

cuda/private/toolchain_configs/clang.bzl

@@ -1,9 +1,9 @@
 load("@bazel_skylib//lib:paths.bzl", "paths")
-load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain")
+load("@bazel_tools//tools/build_defs/cc:action_names.bzl", CC_ACTION_NAMES = "ACTION_NAMES")
+load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain", "use_cpp_toolchain")


cloudhan · 2024-09-11T10:51:00Z

cuda/private/toolchain_configs/clang.bzl

 load("//cuda/private:action_names.bzl", "ACTION_NAMES")
 load("//cuda/private:artifact_categories.bzl", "ARTIFACT_CATEGORIES")
 load("//cuda/private:providers.bzl", "CudaToolchainConfigInfo", "CudaToolkitInfo")
-load("//cuda/private:toolchain.bzl", "use_cpp_toolchain")


cloudhan · 2024-09-11T10:51:11Z

cuda/private/toolchain_configs/nvcc.bzl

 load("//cuda/private:action_names.bzl", "ACTION_NAMES")
 load("//cuda/private:artifact_categories.bzl", "ARTIFACT_CATEGORIES")
 load("//cuda/private:providers.bzl", "CudaToolchainConfigInfo", "CudaToolkitInfo")
-load("//cuda/private:toolchain.bzl", "use_cpp_toolchain")


cloudhan · 2024-09-11T10:51:19Z

cuda/private/toolchain_configs/nvcc.bzl

@@ -1,9 +1,9 @@
 load("@bazel_skylib//lib:paths.bzl", "paths")
-load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain")
+load("@bazel_tools//tools/build_defs/cc:action_names.bzl", CC_ACTION_NAMES = "ACTION_NAMES")
+load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain", "use_cpp_toolchain")


cloudhan · 2024-09-11T10:51:31Z

cuda/private/toolchain_configs/nvcc_msvc.bzl

@@ -1,9 +1,9 @@
 load("@bazel_skylib//lib:paths.bzl", "paths")
-load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain")
+load("@bazel_tools//tools/build_defs/cc:action_names.bzl", CC_ACTION_NAMES = "ACTION_NAMES")
+load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain", "use_cpp_toolchain")


cloudhan · 2024-09-11T10:51:40Z

cuda/private/toolchain_configs/nvcc_msvc.bzl

 load("//cuda/private:action_names.bzl", "ACTION_NAMES")
 load("//cuda/private:artifact_categories.bzl", "ARTIFACT_CATEGORIES")
 load("//cuda/private:providers.bzl", "CudaToolchainConfigInfo", "CudaToolkitInfo")
-load("//cuda/private:toolchain.bzl", "use_cpp_toolchain")


replace accesses to cc_toolchain.compiler_executable with calls to cc…

aa4802c

…_toolchain.get_tool_for_action()

hypdeb requested review from cloudhan, jsharpe and ryanleary as code owners September 8, 2024 19:26

hypdeb marked this pull request as draft September 8, 2024 19:31

hypdeb and others added 6 commits September 8, 2024 21:47

get_tool_for_action is in cc_common

1a3a6bf

passing feature configuration

6baef98

missing load

33ad389

usage of configure_features

35d2090

correct call to configure_features

d297ef0

experimenting

6d31409

added required configuration fragment

b50e8ad

hypdeb marked this pull request as ready for review September 9, 2024 14:22

cloudhan reviewed Sep 10, 2024

View reviewed changes

cuda/private/toolchain.bzl Outdated Show resolved Hide resolved

cloudhan reviewed Sep 10, 2024

View reviewed changes

docs/developer_docs.bzl Outdated Show resolved Hide resolved

hypdeb added 2 commits September 10, 2024 12:48

revert unnecessary changes

4745c07

revert unnecessary changes

f16c4bd

Merge branch 'main' into use_get_tool_for_action

14bb720

cloudhan requested changes Sep 11, 2024

View reviewed changes

remove changes incompatible with older versions of Bazel

eb5d156

cloudhan approved these changes Sep 11, 2024

View reviewed changes

cloudhan merged commit 4dd02f0 into bazel-contrib:main Sep 11, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace accesses to cc_toolchain.compiler_executable with calls to cc_toolchain.get_tool_for_action() #274

replace accesses to cc_toolchain.compiler_executable with calls to cc_toolchain.get_tool_for_action() #274

hypdeb commented Sep 8, 2024 •

edited

Loading

hypdeb commented Sep 8, 2024 •

edited

Loading

hypdeb commented Sep 9, 2024

hypdeb commented Sep 9, 2024

hypdeb commented Sep 9, 2024

cloudhan commented Sep 10, 2024 •

edited

Loading

cloudhan commented Sep 10, 2024

hypdeb commented Sep 10, 2024

cloudhan commented Sep 10, 2024

cloudhan commented Sep 10, 2024

cloudhan Sep 11, 2024

hypdeb Sep 11, 2024

cloudhan Sep 11, 2024

cloudhan Sep 11, 2024

cloudhan Sep 11, 2024

cloudhan Sep 11, 2024

cloudhan Sep 11, 2024

cloudhan Sep 11, 2024

cloudhan Sep 11, 2024

cloudhan Sep 11, 2024

cloudhan Sep 11, 2024

		@@ -1,9 +1,9 @@
		load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain")
		load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain", "use_cpp_toolchain")

		@@ -1,10 +1,10 @@
		load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain")
		load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain", "use_cpp_toolchain")

replace accesses to cc_toolchain.compiler_executable with calls to cc_toolchain.get_tool_for_action() #274

replace accesses to cc_toolchain.compiler_executable with calls to cc_toolchain.get_tool_for_action() #274

Conversation

hypdeb commented Sep 8, 2024 • edited Loading

hypdeb commented Sep 8, 2024 • edited Loading

hypdeb commented Sep 9, 2024

hypdeb commented Sep 9, 2024

hypdeb commented Sep 9, 2024

cloudhan commented Sep 10, 2024 • edited Loading

cloudhan commented Sep 10, 2024

hypdeb commented Sep 10, 2024

cloudhan commented Sep 10, 2024

cloudhan commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypdeb commented Sep 8, 2024 •

edited

Loading

hypdeb commented Sep 8, 2024 •

edited

Loading

cloudhan commented Sep 10, 2024 •

edited

Loading