Skip to content

Add ccache stats reporting for all artifact builds#42077

Closed
murgatroid99 wants to merge 4 commits into
grpc:masterfrom
murgatroid99:distribtest_ccache_stats
Closed

Add ccache stats reporting for all artifact builds#42077
murgatroid99 wants to merge 4 commits into
grpc:masterfrom
murgatroid99:distribtest_ccache_stats

Conversation

@murgatroid99

Copy link
Copy Markdown
Member

This will give us signal about the impact of ccache on the builds in the distribution tests. The code is copied from #42033.


cp -r grpc-*.tgz "${ARTIFACTS_OUT}"/

if [ -x "$(command -v ccache)" ]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sergiitk

Copy link
Copy Markdown
Member

/gemini review


cp -r grpc-*.tgz "${ARTIFACTS_OUT}"/

if [ -x "$(command -v ccache)" ]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may also want to compress it to something like

[ -x "$(command -v ccache)" ] && ccache --show-stats || true

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this, but shellcheck rejected it so I went back to the form I started with.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +28 to +31
if [ -x "$(command -v ccache)" ]
then
ccache --show-stats || true
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The pear package command typically only bundles source files into an archive and does not perform any C/C++ compilation. Since no compilation occurs within this script, reporting ccache statistics here is likely unnecessary and will not provide useful signal regarding build performance.

Comment on lines +36 to +39
if [ -x "$(command -v ccache)" ]
then
ccache --show-stats || true
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
if [ -x "$(command -v ccache)" ]
then
ccache --show-stats || true
fi
[ -x "$(command -v ccache)" ] && ccache --show-stats || true

Comment on lines +331 to +334
if [ -x "$(command -v ccache)" ]
then
ccache --show-stats || true
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider using a more concise one-liner for reporting optional statistics to keep the script clean.

Suggested change
if [ -x "$(command -v ccache)" ]
then
ccache --show-stats || true
fi
[ -x "$(command -v ccache)" ] && ccache --show-stats || true

Comment on lines +63 to +66
if [ -x "$(command -v ccache)" ]
then
ccache --show-stats || true
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
if [ -x "$(command -v ccache)" ]
then
ccache --show-stats || true
fi
[ -x "$(command -v ccache)" ] && ccache --show-stats || true

Comment on lines +26 to +29
if [ -x "$(command -v ccache)" ]
then
ccache --show-stats || true
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This script only performs file copying (cp) of pre-built artifacts and does not involve any compilation. Reporting ccache statistics here is unnecessary as it won't reflect any activity from this specific build phase.

asheshvidyut pushed a commit to asheshvidyut/grpc that referenced this pull request Apr 23, 2026
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
asheshvidyut pushed a commit to a-detiste/grpc that referenced this pull request Jun 10, 2026
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants