Repository: bazelbuild/bazel
Issue: BAZEL_CURRENT_REPOSITORY double escaped when using param files
Labels: type: bug
team-Rules-CPP
untriaged

### Description of the bug:


When using --features=compiler_param_file to compile a C file, the string added to the param file is "double escaped" so that the result is 

`/DBAZEL_CURRENT_REPOSITORY=\"\"` instead of `/DBAZEL_CURRENT_REPOSITORY=""`

This does not appear to cause an issue in the clang/gcc compilation, but causes my toolchain with a non-standard compiler (based on clang) to crash. My compiler can't handle /"/" as a define.

Without --features=compiler_param_file set (and using the -s flag) , I can see that the command line is correctly escaped as `/DBAZEL_CURRENT_REPOSITORY=""` on the command line

The param file should directly mimic the non-param file build, where the the string is properly escaped.

### Which category does this issue belong to?


C++/Objective-C Rules, External Dependency

### What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.


Clone the Examples from https://github.com/bazelbuild/examples

change directory to cpp-tutorial/stage3

Run `bazel build //main:hello-world --features=compiler_param_file -s --compiler=clang-cl`

Observe the .params file at cpp-tutorial\stage3\bazel-out\x64_windows-fastbuild\bin\main\_objs\hello-greet\hello-greet.obj.params

`bazel clean`, then run `bazel build //main:hello-world -s --compiler=clang-cl` to compare BAZEL_CURRENT_REPOSITORY between the two runs

### Which operating system are you running Bazel on?


Windows

### What is the output of `bazel info release`?


release 6.4.0

### If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.


_No response_

### What's the output of `git remote get-url origin; git rev-parse master; git rev-parse HEAD` ?


_No response_

### Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.


_No response_

### Have you found anything relevant by searching the web?

It appears the problem was introduced as part of #16216

### Any other information, logs, or outputs that you want to share?

_No response_

Issue comments:
`/DBAZEL_CURRENT_REPOSITORY=""` actually isn't the escaped form, the value of this preprocessor variable is really meant to be an empty string *literal*, not the empty string.

This class in Bazel is responsible for the escaping and mentions double quotes as special characters that `gcc` and `clang` require to be escaped: https://github.com/bazelbuild/bazel/blob/818c5c8693c43fe490c9f6b2c05149eb8f45cf52/src/main/java/com/google/devtools/build/lib/util/GccParamFileEscaper.java#L32

Since the newline character itself can appear in arguments, there has to be some kind of encoding scheme in parameter files that doesn't apply to command-line arguments. What kind of scheme does your compiler support, e.g., how would it represent this literal argument in the param file?

```
/DMY_VALUE_WITH_NEWLINE="fooo
bar"
```
Thanks for the details.

It appears my compiler is doing something with the first and last " in the param files, so to achieve an empty string literal, it would want 
`-DBAZEL_CURRENT_REPOSITORY="\"\""` 
in the param file.

I can file a bug with the compiler vendor, but this will take months to get fixed that way. Is there any workaround to disable the definiton BAZEL_CURRENT_REPOSITORY in a custom toolchain? I do see that this would also be an issue with the redacted_dates feature for \_\_DATE\_\_, \_\_TIMESTAMP\_\_, and  \_\_TIME__ defines as well, but our "workaround" is to disable that feature (Its not needed anyways for us).
While one could add a feature that disables the addition of `BAZEL_CURRENT_REPOSITORY`, that could end up breaking rulesets and tools from third parties that rely on runfiles libraries and thus this variable. That doesn't sound like a great feature to have since the problem is clearly unrelated to this particular variable.

Alternatives I see:
1. Use a custom Bazel binary until this is fixed in the compiler. This isn't difficult, you just have to modify the line https://github.com/bazelbuild/bazel/blob/a3c677dfea2de636a719d50345a5a97af96fae60/src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl#L971 and then build `bazel build -c opt //src:bazel`. The C++ rules don't exist outside the Bazel binary yet, otherwise you could just load a custom version of `rules_cc`.
2. In your C/C++ toolchain, wrap the compiler into a binary or shell script that fixes up the param file before invoking the compiler with it (see https://github.com/bazelbuild/apple_support/blob/master/crosstool/wrapped_clang.cc for an example that used to be part of Bazel itself).
3. If this quoting style is common enough and not just a bug, maybe support for it could be added to the C++ rules.

Would any of that work for you?

CC @buildbreaker2021 who might have more ideas
The only input I might add is that out of the options @fmeum has listed my favorite would be the second one.
You will have to write shell script once and forget about the bug until it is fixed.
Sharing my opinion on that topic since it poped up on our end as well:

- Adding a define  to a compilation might not affect the final binary - but it could. From saftey perspective it is a problem. 
- In some industry sectors where you have to track each single define, compiler option/flag to pass an assesment, this will become a problem and therefore we decided to not upgrade to Bazel 6.0.0 +. 
- We (internal) still need to make a decision if we want to go on long term with a custom bazel binary, which would be not a one time effort in case this behaviour will be not changed.

Therefore I was happy to find this thread. Coudln't we add a feature that explicitly need to be enabled to overcome that this define will be set ? 
In my original proposal for runfiles handling, I proposed to add the define only to targets that directly depend on the runfiles library in `@bazel_tools//tools/cpp/runfiles`. I could see that being a better solution than introducing a feature.

@buildbreaker2021 What do you think? It would be a slightly incompatible change.
@bazel-io flag
> While one could add a feature that disables the addition of `BAZEL_CURRENT_REPOSITORY`, that could end up breaking rulesets and tools from third parties that rely on runfiles libraries and thus this variable. That doesn't sound like a great feature to have since the problem is clearly unrelated to this particular variable.

Yes, I agree with all this. However, since my team is developing an embedded application, we do not rely on runfiles, so disabling this _should_ not effect our implementation, but I do understand the thought here.

> 
> Alternatives I see:
> 
> 1. Use a custom Bazel binary until this is fixed in the compiler. This isn't difficult, you just have to modify the line https://github.com/bazelbuild/bazel/blob/a3c677dfea2de636a719d50345a5a97af96fae60/src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl#L971
>     and then build `bazel build -c opt //src:bazel`. The C++ rules don't exist outside the Bazel binary yet, otherwise you could just load a custom version of `rules_cc`.

This is probably the "quick" solution that I will implement if I can't get the command line length down below where a param file is required.

> 2. In your C/C++ toolchain, wrap the compiler into a binary or shell script that fixes up the param file before invoking the compiler with it (see https://github.com/bazelbuild/apple_support/blob/master/crosstool/wrapped_clang.cc for an example that used to be part of Bazel itself).

And this will be the longer term solution we will target if needed.

> 3. If this quoting style is common enough and not just a bug, maybe support for it could be added to the C++ rules.

Evaluating our other compilers, it seems that this quoting is unique to one, so I don't think option 3 is the right path for Bazel to take.


> In my original proposal for runfiles handling, I proposed to add the define only to targets that directly depend on the runfiles library in `@bazel_tools//tools/cpp/runfiles`. I could see that being a better solution than introducing a feature.
> 
> @buildbreaker2021 What do you think? It would be a slightly incompatible change.

However, I think this is a more correct solution, which would also fix my issue.

@bazel-io fork 7.0.0
@lwelch @McAmun If possible, could you check whether https://github.com/bazelbuild/bazel/pull/20388 fixes the issue for your builds? It should also be cherry-pickable to Bazel 6 (but not 5).
@fmeum, I cherry-picked #20388 on top of 6.4.0 and it meets my needs.

I'm not sure how easy/hard releasing patch is, but is there any chance of getting this as a patch on top of any 6.x version?


> @fmeum, I cherry-picked this on top of 6.4.0 and it meets my needs.
> 
> I'm not sure how easy/hard releasing patch is, but is there any chance of getting this as a patch on top of any 6.x version?

cc: @Wyverald
@iancha1992 As this is a (mildly) backwards incompatible change, we could either cherry-pick it into 7.0.0 or may have to wait for Bazel 8. 

It could be argued that we never promised that `BAZEL_CURRENT_REPOSITORY` is available everywhere as we only document it in the C++ runfiles library docs, which *may* make it okay to cherry-pick into 7.1.0.
> It could be argued that we never promised that BAZEL_CURRENT_REPOSITORY is available everywhere as we only document it in the C++ runfiles library docs, which may make it okay to cherry-pick into 7.1.0.

Yes, I think https://github.com/bazelbuild/bazel/pull/20388 is more of a fix for an existing issue instead of a breaking change of an existing feature. And I guess the actual risk of breaking user should be small and easy to fix anyway? Let's cherry-pick it into 7.1.0.
Sounds good!
A fix for this issue has been included in [Bazel 7.0.0 RC6](https://releases.bazel.build/7.0.0/rc6/index.html). Please test out the release candidate and report any issues as soon as possible. Thanks!
> A fix for this issue has been included in [Bazel 7.0.0 RC6](https://releases.bazel.build/7.0.0/rc6/index.html). Please test out the release candidate and report any issues as soon as possible. Thanks!

I tested [Bazel 7.0.0 RC6] and it also resolved this issue. Thanks.

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl
index ea967bc0976a14..39479fe3a3ff3a 100644
--- a/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl
+++ b/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl
@@ -634,7 +634,7 @@ def cc_binary_impl(ctx, additional_linkopts):
         cc_toolchain = cc_toolchain,
         user_compile_flags = cc_helper.get_copts(ctx, feature_configuration, additional_make_variable_substitutions),
         defines = cc_helper.defines(ctx, additional_make_variable_substitutions),
-        local_defines = cc_helper.local_defines(ctx, additional_make_variable_substitutions) + cc_helper.get_local_defines_for_runfiles_lookup(ctx),
+        local_defines = cc_helper.local_defines(ctx, additional_make_variable_substitutions) + cc_helper.get_local_defines_for_runfiles_lookup(ctx, ctx.attr.deps),
         system_includes = cc_helper.system_include_dirs(ctx, additional_make_variable_substitutions),
         private_hdrs = cc_helper.get_private_hdrs(ctx),
         public_hdrs = cc_helper.get_public_hdrs(ctx),
diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl
index a0ceefdae959e7..8a7468b78fc0f7 100644
--- a/src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl
+++ b/src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl
@@ -965,8 +965,13 @@ def _is_stamping_enabled_for_aspect(ctx):
         stamp = ctx.rule.attr.stamp
     return stamp
 
-def _get_local_defines_for_runfiles_lookup(ctx):
-    return ["BAZEL_CURRENT_REPOSITORY=\"{}\"".format(ctx.label.workspace_name)]
+_RUNFILES_LIBRARY_TARGET = Label("@bazel_tools//tools/cpp/runfiles")
+
+def _get_local_defines_for_runfiles_lookup(ctx, all_deps):
+    for dep in all_deps:
+        if dep.label == _RUNFILES_LIBRARY_TARGET:
+            return ["BAZEL_CURRENT_REPOSITORY=\"{}\"".format(ctx.label.workspace_name)]
+    return []
 
 # This should be enough to assume if two labels are equal.
 def _are_labels_equal(a, b):
diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_library.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_library.bzl
index a396bb5fe137a5..5902c0d0875328 100755
--- a/src/main/starlark/builtins_bzl/common/cc/cc_library.bzl
+++ b/src/main/starlark/builtins_bzl/common/cc/cc_library.bzl
@@ -57,7 +57,7 @@ def _cc_library_impl(ctx):
         feature_configuration = feature_configuration,
         user_compile_flags = cc_helper.get_copts(ctx, feature_configuration, additional_make_variable_substitutions),
         defines = cc_helper.defines(ctx, additional_make_variable_substitutions),
-        local_defines = cc_helper.local_defines(ctx, additional_make_variable_substitutions) + cc_helper.get_local_defines_for_runfiles_lookup(ctx),
+        local_defines = cc_helper.local_defines(ctx, additional_make_variable_substitutions) + cc_helper.get_local_defines_for_runfiles_lookup(ctx, ctx.attr.deps + ctx.attr.implementation_deps),
         system_includes = cc_helper.system_include_dirs(ctx, additional_make_variable_substitutions),
         copts_filter = cc_helper.copts_filter(ctx, additional_make_variable_substitutions),
         purpose = "cc_library-compile",
diff --git a/tools/cpp/runfiles/runfiles_src.h b/tools/cpp/runfiles/runfiles_src.h
index 242b834856553f..93e9da198d330d 100644
--- a/tools/cpp/runfiles/runfiles_src.h
+++ b/tools/cpp/runfiles/runfiles_src.h
@@ -48,6 +48,8 @@
 //         ...
 //
 //      The code above creates a Runfiles object and retrieves a runfile path.
+//      The BAZEL_CURRENT_REPOSITORY macro is available in every target that
+//      depends on the runfiles library.
 //
 //      The Runfiles::Create function uses the runfiles manifest and the
 //      runfiles directory from the RUNFILES_MANIFEST_FILE and RUNFILES_DIR


Test patch:
diff --git a/src/test/shell/bazel/cc_integration_test.sh b/src/test/shell/bazel/cc_integration_test.sh
index 72a3ae6025e384..2512dc300c3bb5 100755
--- a/src/test/shell/bazel/cc_integration_test.sh
+++ b/src/test/shell/bazel/cc_integration_test.sh
@@ -1519,19 +1519,26 @@ cc_library(
   name = "library",
   srcs = ["library.cpp"],
   hdrs = ["library.h"],
+  implementation_deps = ["@bazel_tools//tools/cpp/runfiles"],
   visibility = ["//visibility:public"],
 )
 
 cc_binary(
   name = "binary",
   srcs = ["binary.cpp"],
-  deps = [":library"],
+  deps = [
+    ":library",
+    "@bazel_tools//tools/cpp/runfiles",
+  ],
 )
 
 cc_test(
   name = "test",
   srcs = ["test.cpp"],
-  deps = [":library"],
+  deps = [
+    ":library",
+    "@bazel_tools//tools/cpp/runfiles",
+  ],
 )
 EOF
 
@@ -1573,13 +1580,19 @@ EOF
 cc_binary(
   name = "binary",
   srcs = ["binary.cpp"],
-  deps = ["@//pkg:library"],
+  deps = [
+    "@//pkg:library",
+    "@bazel_tools//tools/cpp/runfiles",
+  ],
 )
 
 cc_test(
   name = "test",
   srcs = ["test.cpp"],
-  deps = ["@//pkg:library"],
+  deps = [
+    "@//pkg:library",
+    "@bazel_tools//tools/cpp/runfiles",
+  ],
 )
 EOF