bazel icon indicating copy to clipboard operation
bazel copied to clipboard

Add command profile to the BEP under a fixed name

Open fmeum opened this issue 9 months ago • 5 comments

If users specify a custom profile location with --profile, the file is added to the BES under its basename. This makes it harder for BES consumers to pick out the profile. Instead, always announce it under its default name command.profile.gz or command.profile if uncompressed. This additionally allows consumers to detect compression based on just the file name.

fmeum avatar May 13 '24 12:05 fmeum

@tjgq Could you review this? It's a small change that seems like all upside to me, but curious to learn what you think about it.

fmeum avatar May 13 '24 12:05 fmeum

To be honest, I can see it being confusing both ways (from the standpoint of someone setting --profile=foo, how would they guess that command.profile.gz is what it's called in the BEP)?

@meisterT thoughts?

tjgq avatar May 13 '24 13:05 tjgq

I think end users would not be consuming the profile data from BEP directly. Folks often consume it via some sort of automation system, like a Build Event Service implementation.

With more files being added to the build log tools, these service implementations might have a harder time telling which log is the timing profile data. The current workflow requires folks to parse the --profile flag value in the canonical command line event, then parse the file path (Windows vs Unix) to determine the base filename.

Using a constant name would help remove all these complications in the future.

sluongng avatar May 13 '24 13:05 sluongng

FYI, I expect a couple of tests have to be amended:

https://cs.opensource.google/bazel/bazel/+/master:src/test/shell/bazel/remote/remote_build_event_uploader_test.sh;l=543;drc=416c6c6c129d15e88e1ab9003150d4c1fb04fd80 https://cs.opensource.google/bazel/bazel/+/master:src/test/shell/bazel/remote/remote_build_event_uploader_test.sh;l=563;drc=416c6c6c129d15e88e1ab9003150d4c1fb04fd80

tjgq avatar May 13 '24 13:05 tjgq

I updated the tests and added a new one to cover the uncompressed format case.

fmeum avatar May 13 '24 14:05 fmeum