Skip to content

Cache sccache preprocessor and toolchain dirs across runs#408

Merged
trxcllnt merged 14 commits intobranch-25.10from
fea/preprocessor-caching
Aug 11, 2025
Merged

Cache sccache preprocessor and toolchain dirs across runs#408
trxcllnt merged 14 commits intobranch-25.10from
fea/preprocessor-caching

Conversation

@trxcllnt
Copy link
Copy Markdown
Contributor

@trxcllnt trxcllnt commented Aug 8, 2025

This PR caches the .cache/{sccache,sccache-dist-client} dirs across CI runs so builds are accelerated by sccache's preprocessor cache mode.

@trxcllnt trxcllnt requested a review from a team as a code owner August 8, 2025 14:44
@trxcllnt trxcllnt requested a review from bdice August 8, 2025 14:44
@trxcllnt trxcllnt added non-breaking Introduces a non-breaking change improvement Improves an existing functionality labels Aug 8, 2025
echo "HAS_DEVCONTAINER=false" >> "${GITHUB_ENV}"
HAS_DEVCONTAINER=false
if test -f "repo/.devcontainer/cuda${CUDA_VER}-${PACKAGER}/devcontainer.json"; then
HAS_DEVCONTAINER=true
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.

If this is false because there’s no devcontainer, what does the “build devcontainer” workflow do? How does the conditional help us?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If there's no devcontainer, we want to do nothing without failing the build. GHA has no "exit this workflow early without error" function, so this sets a boolean that all the steps after this check and skip if it's not true.

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.

If there's no devcontainer, we want to do nothing without failing the build.

Why would we not want to fail the build in that case?

I don't think we should do this... it could silently cover up actual problems. For example, if any repo deviates from the standard naming convention we have (cuda${CUDA_VER}-${PACKAGER}/devcontainer.json), today you get a big loud CI failure telling you about that.

With this check, that'd turn into this job just silently being a no-op, which might be missed.

Repos without devcontainers should just not use this workflow, and this workflow should fail outright if what it needs isn't found.

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 talked offline about this... this is not new behavior, it's only showing up here because of the change to git cloning that now writes the repo to repo/ (changing the file path passed to test -f).

Approving this, but I do think we should remove this HAS_DEVCONTAINER=false behavior in a follow-up.

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.

Great. I agree we should fail loudly here, and remove this in a follow-up.

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.

remove this in a follow-up

Proposing that in #487

@trxcllnt trxcllnt merged commit a2832ab into branch-25.10 Aug 11, 2025
2 checks passed
@trxcllnt trxcllnt deleted the fea/preprocessor-caching branch August 11, 2025 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants