Add flag --experimental_remote_build_event_upload

[coeuvre]

Add flag --experimental_remote_build_event_upload which controls the way Bazel uploads files referenced in BEP to remote cache.

It defaults to all which maintains current behaviour: Bazel uploads all local files referenced by BEP to remote cache and convert their paths to bytestream://.... Additionally, --incompatible_remote_build_event_upload_respect_no_cache can be set to avoid uploading outputs that are generated by "non-remote-cachable" spawns.

If set to minimal, local outputs are not uploaded to the remote cache, except for files that are important to the consumers of BES (e.g. test logs and timing profile). Paths for files that are already uploaded to the remote cache are converted.

--incompatible_remote_build_event_upload_respect_no_cache is deprecated in favour of this new flag.

Fixes #16151.

[brentleyjones]

Does it make sense to mention the paths here? And if so, can the explanation be expanded, since what are they being converted to, and what does it have to do with uploading? (I know the answers to these, but thinking more for the documentation.)

[coeuvre]

Updated doc.

[brentleyjones]

So does --experimental_remote_build_event_upload=minimal also imply --incompatible_remote_build_event_upload_respect_no_cache? This deprecation should probably mention the minimal value?

[coeuvre]

So does --experimental_remote_build_event_upload=minimal also imply --incompatible_remote_build_event_upload_respect_no_cache?

No. I think with --experimental_remote_build_event_upload=minimal, there is no need for incompatible_remote_build_event_upload_respect_no_cache, right?

Updated message to mention minimal.

[coeuvre]

@brentleyjones can you give this patch a try with your use cases? I think it's good enough but I might miss something.

[brentleyjones]

Do we need to mention the path behavior of all? Or adjust it to be agnostic of the setting (since the previous sentence already states what is or is not being uploaded)?

[coeuvre]

Done.

[brentleyjones]

@coeuvre Will do.

[brentleyjones]

Do you want to mention in the PR description that this fixes #16151? (I'm still testing that though.)

[brentleyjones]

This works in my testing.

I've asked a couple others to verify as well though.

[erikkerber]

I tested as well and works well for our use case and what we were seeing.

[BalestraPatrick]

Looks like this is working for us as well. I tested both on 5.3 without this flag to reproduce that bes-upload uploads various MBs of artifacts. Then on this PR with the flag set bes-upload doesn't upload the same artifacts, and of course on this PR without this flag the outputs are still uploaded. 👍

[brentleyjones]

@coeuvre @meteorcloudy This has been great in our usage. Can we have the name become non-experimental, and the default changed to minimal, for 7.0?

[coeuvre]

SGTM.

[brentleyjones]

@meteorcloudy I don't think anything related to the implementation has changed in 7.0, the "it's worked great" has been 6.1.1 for me. Since a name change isn't a breaking change, can I include that part in 6.2? So I would do two PRs, one to change the name (and cherry-pick that) and one to flip the value (for 7.0 only)?

I'm fine either way, I just want people to discover and use this great feature 😊.

[meteorcloudy]

SGTM, and I believe we have the oldName attribute for a flag, so that users still depend on --experimental_remote_build_event_upload won't be broken at all.

For example,

bazel/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java

Line 201 in 9f2cc56

oldName = "experimental_enable_bzlmod",

[brentleyjones]

#17972

[brentleyjones]

#17988

[lijunsong]

@brentleyjones

When I set --experimental_remote_build_event_upload=minimal, the artifacts are not uploaded as expected, but the artifacts reference links are all bytestream instead of file even for those targets that are cached. Is this expected?

To reproduce this, you'll need to enable --remote_executor, and then:

$ echo "int main(){}" > main.cc
$ echo 'cc_binary(name="main", srcs=["main.cc"])' > BUILD

Build with old flag

then build

bazel build --build_event_json_file=/tmp/before.json --incompatible_remote_build_event_upload_respect_no_cache :main

The event contains

{"id":{"namedSet":{"id":"0"}},"namedSetOfFiles":{"files":[{"name":"y4/main","uri":"file:///home/junsong-li/..../execroot/gibraltar/bazel-out/x86_64-opt-out/bin/y4/main","pathPrefix":["bazel-out","x86_64-opt-out","bin"],"digest":"2095f05a9e2118c2a42b6ef03d8e57b5fd7701fab737d0c784e2721a88d89562","length":"7448"}]}}

Build with new flag

$ echo >> main.cc

and then add the flag

bazel build --build_event_json_file=/tmp/after.json --experimental_remote_build_event_upload=minimal  :main

The event contains

{"id":{"namedSet":{"id":"0"}},"namedSetOfFiles":{"files":[{"name":"y4/main","uri":"bytestream://remote_execution.hostname:port/remote-execution/blobs/be3a30bd7cf1aaf2e77fd492826e35b3a445e43aebf5f3de842ebbcad745096d/7448","pathPrefix":["bazel-out","x86_64-opt-out","bin"],"digest":"be3a30bd7cf1aaf2e77fd492826e35b3a445e43aebf5f3de842ebbcad745096d","length":"7448"}]}}

The reference doesn't exist

$ /tmp/tools_remote/bazel-bin/remote_client --remote_cache=remote_execution.hostname:port --remote_timeout=10 cat --digest be3a30bd7cf1aaf2e77fd492826e35b3a445e43aebf5f3de842ebbcad745096d/7448 --file=/tmp/myfile
Error: com.google.devtools.build.remote.client.CacheNotFoundException: Missing digest: hash: "be3a30bd7cf1aaf2e77fd492826e35b3a445e43aebf5f3de842ebbcad745096d"
size_bytes: 7448

So in sum,

1. The before.json contains file:// reference
   2.The after.json contains bytestream:// reference even though the blob doesn't exist.

Edit 1: this is bazel 6.2.1
Edit 2: bazelrc needs to turn off remote linking so that the main binary is linked locally to test bes upload.

build --modify_execution_info=CppLink=+no-remote

to suspend upload of the linking result.

[lijunsong]

Oh no, after #16999, BES always uses bytestream.

[brentleyjones]

You can use --nobuild_event_json_file_path_conversion to get the file paths, right?