Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix caching of target dir for fuzz project #96

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

kdarkhan
Copy link
Collaborator

@kdarkhan kdarkhan commented Oct 4, 2023

Currently caching does not work for fuzz job which can be verified by looking at Github workflow outputs.

The main cause of the issue is the config workspaces: fuzz -> target. The fuzz target does not use a proper workspace but a separate crate. There is a pending issue discussing that.

The rust-cache action is executed in the root of the project and is not able to locate fuzz directory by workspace name.

I decided to list the directories to cache instead of relying on target dir detection which seems to work.

I also fixed the path fuzz/corpus to i18n-helpers/fuzz/corpus.

I adjusted the caching condition save-if to cache only on main builds. I think this makes sense because we don't need to cache PRs which might not get merged or get updated multiple times during review.

Currently caching does not work for `fuzz` job which can
be verified by looking at Github workflow exec outputs.

The main cause of the issue is the config `workspaces: fuzz -> target`.
The fuzz target does not use a proper workspace but a separate
crate. There is [a pending issue](rust-fuzz/cargo-fuzz#345)
discussing that.

The `rust-cache` action is executed in the root of the project and is
not able to locate `fuzz` directory by workspace name.

I decided to list the directories to cache instead of relying
on target dir detection which seems to work.

I also fixed the path `fuzz/corpus` to `i18n-helpers/fuzz/corpus`.

I adjusted the caching condition `save-if` to cache only on `main`
builds. I think this makes sense because we don't need to cache PRs
which might not get merged or get updated multiple times during review.
Copy link
Collaborator

@mgeisler mgeisler left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good. Before, I let the Cargo-aware caching handle the fuzz directory and used the regular caching system for the plain text fuzz corpus. Using the Cargo caching for both ought to work as well.

@mgeisler mgeisler merged commit 941a188 into google:main Oct 5, 2023
5 checks passed
@mgeisler
Copy link
Collaborator

mgeisler commented Oct 5, 2023

@sakex, I believe we forgot this in #86.

@kdarkhan kdarkhan deleted the fuzz-caching-fix branch October 5, 2023 09:04
@sakex
Copy link
Collaborator

sakex commented Oct 5, 2023

Oh sorry I missed this and thanks for the fix

@mgeisler
Copy link
Collaborator

mgeisler commented Oct 5, 2023

Oh sorry I missed this and thanks for the fix

I only mentioned it so that we're now three people who know about the cache path 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants