Convert paths, arguments, and env vars to Unicode for remote execution.

[jmillikin]

Fixes #14381

I didn't touch the Platform construction because it seems to be used in parts of Bazel that aren't specific to remote execution. Maybe it'd be fine to change too? Comments welcomed.

[coeuvre]

cc @tjgq

[tjgq]

It's not entirely obvious to me that Platform has the same issue (its contents originate from command line arguments, not Starlark). Adding a test would be the way to be sure, but feel free to punt on it, since non-ASCII values are probably not as common there.

FYI, while this PR is most definitely welcome, I suspect it doesn't completely fix #14381, since that bug report also mentions an encoding issue in bazel aquery. So feel free to mention it, but I'd suggest leaving it open for now.

[jmillikin]

FYI, while this PR is most definitely welcome, I suspect it doesn't completely fix #14381, since that bug report also mentions an encoding issue in bazel aquery. So feel free to mention it, but I'd suggest leaving it open for now.

I think the bad bazel aquery output might be the same bug, or one nearly identical. It's assembling a Unicode string for the output, and doesn't convert Starlark strings to Unicode before formatting them.

I've tweaked this PR so it also fixes aquery, using the new helper functions.

It's not entirely obvious to me that Platform has the same issue (its contents originate from command line arguments, not Starlark).

While platform properties can be obtained from CLI flags, they also come from Starlark (the platform rule or exec_properties common attribute). I agree that non-ASCII values in platform properties are probably rare, so not going to worry about it for now, but it might be worth fixing in the future.

For reference, the following property configurations are equivalent:

test_unicode(
    name = "test_unicode",
    exec_properties = {"PROPERTY_あ": "あ"},
)

$ bazel build --remote_default_platform_properties='properties {name:"PROPERTY_あ" value:"あ"}'

$ bazel build --remote_default_exec_properties=PROPERTY_あ=あ

And result in the same (incorrect) Platform message being sent to the remote executor.

platform: {
  properties: {
    name: "PROPERTY_ã\u0081\u0082"
    value: "ã\u0081\u0082"
  }
}

[tjgq]

I've tweaked this PR so it also fixes aquery, using the new helper functions.

Thanks! Do you mind also adding a test for bazel aquery, probably in src/test/shell/integration/aquery_test.sh?

While platform properties can be obtained from CLI flags, they also come from Starlark (the platform rule or exec_properties common attribute). I agree that non-ASCII values in platform properties are probably rare, so not going to worry about it for now, but it might be worth fixing in the future.

Thanks, I wasn't aware of the exec_properties attribute. I agree that we don't have to fix this right now.

[jmillikin]

Thanks! Do you mind also adding a test for bazel aquery, probably in src/test/shell/integration/aquery_test.sh?

Done -- I added a test for bazel aquery --output=text.

Fixing the other aquery output formats (for example --output=textproto) are a bit beyond my depth in terms of Bazel knowledge. They seem to live mostly in skyframe/actiongraph/ and I'm having some trouble figuring out what downstream consumers of their output might depend on exactly matching Bazel internal state.

For those, I've added a DISABLED_ test that would pass if --output=textproto passed along Unicode. Hopefully it's OK to fix that in a separate change.

[jmillikin]

Note on the bazel aquery output: on Windows, reliably writing non-ASCII to stdout requires a specially configured PrintWriter. I got lost trying to figure out how the OutputStream stream gets built, so instead it now escapes non-ASCII characters rather than try to print them. It's a little less pretty in the output, but is still usable for debugging and works on all tested platforms.

Also, apparently, on Windows strings for file paths are Unicode and strings from Starlark are UTF-8. I extended the logic in decodeBytestringUtf8() to try to handle this as best it can.

Here's how the new aquery output looks:

$ cat pkg/BUILD 
genrule(
    name = "bar",
    srcs = glob(["*.txt"]),
    outs = ["bar_out.txt"],
    cmd = "echo unused > $(OUTS)",
)
$ ls pkg
BUILD            ünïcödë.txt
$ bazel aquery --output=text //pkg:bar | grep Inputs
  Inputs: [external/bazel_tools/tools/genrule/genrule-setup.sh, pkg/{U+00FC}n{U+00EF}c{U+00F6}d{U+00EB}.txt]

[tjgq]

Let's go with the standard-ish \uXXXX notation to make the output copy-pastable into a script.

[jmillikin]

Done.

[tjgq]

Thanks, I'm going to import this now. Apologies for my confusion earlier and the review delay (I was out on Friday).