Skip to content
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

Consider adding user_compile_flags to cargo_build_script's get_cc_compile_args_and_env() #3041

Open
bazaah opened this issue Dec 4, 2024 · 0 comments

Comments

@bazaah
Copy link

bazaah commented Dec 4, 2024

While investigating how to build rust *-sys crates under rules_rust with custom c/cxx/ld flags (via the --copt|cxxopt|linkopt bazel build options) I discovered that:

  1. The rules_rust build script wrapper does honor --linkopts (settings are reflected in LDFLAGS)
  2. But seemingly ignores --copt & --cxxopt (settings are not reflected in CFLAGS/CXXFLAGS)

This confused me for a couple hours, but I eventually discovered that seemingly bazel's cc_toolchain (?) does not reflect these in the normal flow of things, and requires one to explicitly pass fragments.cpp.copts + fragments.cpp.cxxopts to cc_common.create_compile_variables() for them to show up in cc_common.get_memory_inefficient_command_line() calls.

This is the patch I have for myself:

cargo/private: add user_compile_flags to honor copt/cxxopt
diff --git a/cargo/private/cargo_build_script.bzl b/cargo/private/cargo_build_script.bzl
index be08907e..f15d5810 100644
--- a/cargo/private/cargo_build_script.bzl
+++ b/cargo/private/cargo_build_script.bzl
@@ -119,7 +119,7 @@ https://github.com/bazelbuild/bazel/issues/15486
     executable = True,
 )

-def get_cc_compile_args_and_env(cc_toolchain, feature_configuration):
+def get_cc_compile_args_and_env(ctx, cc_toolchain, feature_configuration):
     """Gather cc environment variables from the given `cc_toolchain`

     Args:
@@ -135,6 +135,7 @@ def get_cc_compile_args_and_env(cc_toolchain, feature_configuration):
     compile_variables = cc_common.create_compile_variables(
         feature_configuration = feature_configuration,
         cc_toolchain = cc_toolchain,
+        user_compile_flags = ctx.fragments.cpp.copts + ctx.fragments.cpp.cxxopts,
     )
     cc_c_args = cc_common.get_memory_inefficient_command_line(
         feature_configuration = feature_configuration,
@@ -386,7 +387,7 @@ def _cargo_build_script_impl(ctx):
     env["LDFLAGS"] = " ".join(_pwd_flags(link_args))

     # MSVC requires INCLUDE to be set
-    cc_c_args, cc_cxx_args, cc_env = get_cc_compile_args_and_env(cc_toolchain, feature_configuration)
+    cc_c_args, cc_cxx_args, cc_env = get_cc_compile_args_and_env(ctx, cc_toolchain, feature_configuration)
     include = cc_env.get("INCLUDE")
     if include:
         env["INCLUDE"] = include

This will have a knock on effect for anyone who's doing c/cpp and rust development in the same workspace / repo, so it may not be worth the disruption, but it strikes me as odd that rules_rust honors --linkopt (which is also a cpp.fragment), but not the other two.

I'm looking for input from a maintainer about whether this is something that should be fixed, or if not; perhaps clarified in the docs somewhere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant