BAZEL_CURRENT_REPOSITORY double escaped when using param files

[lwelch]

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

[fmeum]

/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:

bazel/src/main/java/com/google/devtools/build/lib/util/GccParamFileEscaper.java

Line 32 in 818c5c8

public final class GccParamFileEscaper extends CharEscaper {

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"

[lwelch]

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

[fmeum]

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

   bazel/src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl

   Line 971 in a3c677d

   return ["BAZEL_CURRENT_REPOSITORY=\"{}\"".format(ctx.label.workspace_name)]

   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

[buildbreaker2021]

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.

[McAmun]

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 ?

[fmeum]

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.

[fmeum]

@bazel-io flag

[lwelch]

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

   bazel/src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl

   Line 971 in a3c677d

   return ["BAZEL_CURRENT_REPOSITORY=\"{}\"".format(ctx.label.workspace_name)]

   
   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.

[iancha1992]

@bazel-io fork 7.0.0

[fmeum]

@lwelch @McAmun If possible, could you check whether #20388 fixes the issue for your builds? It should also be cherry-pickable to Bazel 6 (but not 5).

[lwelch]

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

[iancha1992]

@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

[fmeum]

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

[meteorcloudy]

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

[fmeum]

Sounds good!

[iancha1992]

A fix for this issue has been included in Bazel 7.0.0 RC6. Please test out the release candidate and report any issues as soon as possible. Thanks!

[lwelch]

A fix for this issue has been included in Bazel 7.0.0 RC6. 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.