Add ccache stats reporting for all artifact builds#42077
Conversation
|
|
||
| cp -r grpc-*.tgz "${ARTIFACTS_OUT}"/ | ||
|
|
||
| if [ -x "$(command -v ccache)" ] |
There was a problem hiding this comment.
nit: Just to stay on the safer side, move it closer to the command that actually does the build. As it is, everything will work ok, just to fool-proof ourselves from cases where -e is accidentally removed.
There was a problem hiding this comment.
I don't think this makes sense in the Python script, because it has a bunch of different builds in conditional blocks, so the only way to consistently get all stats is to put it at the end. I made the change in the protoc build script. There aren't any meaningful stats in the other scripts, so I removed the command there.
|
/gemini review |
|
|
||
| cp -r grpc-*.tgz "${ARTIFACTS_OUT}"/ | ||
|
|
||
| if [ -x "$(command -v ccache)" ] |
There was a problem hiding this comment.
We may also want to compress it to something like
[ -x "$(command -v ccache)" ] && ccache --show-stats || trueThere was a problem hiding this comment.
I tried this, but shellcheck rejected it so I went back to the form I started with.
There was a problem hiding this comment.
Code Review
This pull request adds ccache statistics reporting to several artifact build scripts. The reviewer pointed out that reporting these statistics is unnecessary in scripts that only perform packaging or file copying, such as the PHP build and package scripts. For the remaining scripts, the reviewer suggested using a more concise one-liner and noted that ccache might not be effectively capturing data if the environment is not properly configured with compiler symlinks.
| if [ -x "$(command -v ccache)" ] | ||
| then | ||
| ccache --show-stats || true | ||
| fi |
There was a problem hiding this comment.
| if [ -x "$(command -v ccache)" ] | ||
| then | ||
| ccache --show-stats || true | ||
| fi |
There was a problem hiding this comment.
To reduce boilerplate across multiple scripts, consider using a more concise one-liner. Additionally, note that this script does not source tools/internal_ci/helper_scripts/prepare_ccache_symlinks_rc. Unless ccache is already configured to wrap the compiler in the environment's PATH, these statistics may not reflect any actual build activity.
| if [ -x "$(command -v ccache)" ] | |
| then | |
| ccache --show-stats || true | |
| fi | |
| [ -x "$(command -v ccache)" ] && ccache --show-stats || true |
| if [ -x "$(command -v ccache)" ] | ||
| then | ||
| ccache --show-stats || true | ||
| fi |
| if [ -x "$(command -v ccache)" ] | ||
| then | ||
| ccache --show-stats || true | ||
| fi |
There was a problem hiding this comment.
Consider using a more concise one-liner. Also, similar to the protoc artifact build, this script lacks the explicit ccache initialization found in build_artifact_python.sh. If the compiler is not correctly wrapped via PATH symlinks, these statistics may remain empty.
| if [ -x "$(command -v ccache)" ] | |
| then | |
| ccache --show-stats || true | |
| fi | |
| [ -x "$(command -v ccache)" ] && ccache --show-stats || true |
| if [ -x "$(command -v ccache)" ] | ||
| then | ||
| ccache --show-stats || true | ||
| fi |
This will give us signal about the impact of ccache on the builds in the distribution tests. The code is copied from grpc#42033. <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. --> Closes grpc#42077 COPYBARA_INTEGRATE_REVIEW=grpc#42077 from murgatroid99:distribtest_ccache_stats ba85091 PiperOrigin-RevId: 902675367
This will give us signal about the impact of ccache on the builds in the distribution tests. The code is copied from grpc#42033. <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. --> Closes grpc#42077 COPYBARA_INTEGRATE_REVIEW=grpc#42077 from murgatroid99:distribtest_ccache_stats ba85091 PiperOrigin-RevId: 902675367
This will give us signal about the impact of ccache on the builds in the distribution tests. The code is copied from #42033.