Reencode non-ASCII strings in BEP command line and metadata events - #30665
Reencode non-ASCII strings in BEP command line and metadata events#30665oscarthecat wants to merge 3 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
github-actions
Bot
added
team-Rules-Server
fmeum
left a comment
fmeum
left a comment
There was a problem hiding this comment.
The WorkspaceStatus entries being encoded correctly here is actually concerning though as other parts of the system assume that everything is in the internal encoding. If you know where they are being parsed, could you make sure they are retained in internal encoding there and only converted back to Unicode when forming the BES proto?
|
Thanks, that was a good catch — and it turned out to be more than a convention issue.
Changes:
Added |
ed2f768 to
b5fa902
Compare
| } | ||
| return stdoutStream.toString(UTF_8); | ||
| // Keep the output as an internal string, i.e. raw bytes in a Latin-1 String, so that it | ||
| // matches the encoding of the other entries in the status maps and of the bytes written |
There was a problem hiding this comment.
Just say that this uses Bazel's internal string encoding and reference StringEncoding.
| .collect(joining("\n")); | ||
| s += "\n"; | ||
| return s.getBytes(StandardCharsets.UTF_8); | ||
| // The map values are internal strings, i.e. raw bytes in a Latin-1 String, so write them out |
Bazel keeps strings internally as raw bytes in Latin-1 `String`s, whereas protobuf `string` fields hold Unicode. `CommandLineEvent` and `BuildMetadataEvent` wrote internal strings into the BEP protos as is, so a non-ASCII option value, residue entry or build metadata value came out double encoded. Route these values through `StringEncoding#internalToUnicode`, as is already done for other proto fields, e.g. in `BuildLanguageInfoItem`. ASCII values are unaffected. The values are the ones a consumer of the BEP is most likely to display: an invocation started with a non-ASCII `--build_metadata`, `--client_env` or target pattern showed mojibake in the UI rather than what the user had passed in, and there was no way to recover the original bytes from the stream.
The workspace status command output was decoded as UTF-8, which made its entries the only Unicode strings in the status maps, and it was reencoded as UTF-8 on the way into the status files. `WorkspaceStatusAction#parseValues` reads those files as Latin-1, so a non-ASCII value did not survive the round trip and reached `BuildInfoFileWriteAction` mangled. Decode the output as Latin-1 so that it stays in the internal encoding like every other entry, and write the status files without reencoding. The files still hold the exact bytes the command emitted. Convert to Unicode in `BuildInfoEvent`, where the BEP proto is formed and it is actually needed.
b5fa902 to
847a9be
Compare
Rather than restating how the internal encoding represents bytes, name it and point at `StringEncoding`, which documents it.
|
Done, thanks — comments now just name the internal string encoding and point at Pushed as a separate commit (9a6bdb6) so the change is easy to see. Also applied the same simplification to the comment in CI is green and the CLA check is sorted out. Happy to squash or adjust anything else. |
|
@bazel-io fork 9.3.0 |
github-actions
Bot
added
the
community-reviewed
2 participants
Bazel stores strings internally as raw bytes in Latin-1 Strings, while protobuf string fields are Unicode. CommandLineEvent and BuildMetadataEvent wrote internal strings into the BEP protos without reencoding them, so any non-ASCII option value, residue entry or build metadata value ended up double encoded in the BEP.
For example, with a UTF-8 environment variable GITLAB_USER_NAME=王芳兵, the canonical command line reported --client_env=GITLAB_USER_NAME=çè³åµ, and --build_metadata=USER=王芳兵 was mangled the same way. Consumers of the BEP that display these values, such as BuildBuddy, show the mojibake.
Route these values through StringEncoding.internalToUnicode(), as is already done for other proto fields, e.g. in BuildLanguageInfoItem. ASCII values are unaffected. WorkspaceStatus values were already correct, since the workspace status output is decoded as UTF-8 when it is parsed.
Description
Motivation
Build API Changes
No
Checklist
Release Notes
RELNOTES: None