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

cache and restore dumped accounts #104

Conversation

samuelvanderwaal
Copy link
Contributor

@samuelvanderwaal samuelvanderwaal commented Nov 14, 2024

Current workflow does not seem to cache and restore dumped external accounts. I can add a changeset if this should be a patch bump.

Copy link

changeset-bot bot commented Nov 14, 2024

🦋 Changeset detected

Latest commit: 9c9056b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
create-solana-program Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@lorisleiva
Copy link
Member

Hey Sam! Thanks for raising this. It's strange because I thought this would be taken care of by the cache jobs inside the setup actions.

- name: Cache Cargo Dependencies
if: {% raw %}${{ inputs.cargo-cache-key && !inputs.cargo-cache-fallback-key }}{% endraw %}
uses: actions/cache@v4
with:
path: |
~/.cargo/bin/
~/.cargo/registry/index/
~/.cargo/registry/cache/
~/.cargo/git/db/
target/
key: {% raw %}${{ runner.os }}-${{ inputs.cargo-cache-key }}-${{ hashFiles('**/Cargo.lock') }}{% endraw %}
restore-keys: {% raw %}${{ runner.os }}-${{ inputs.cargo-cache-key }}{% endraw %}
- name: Cache Cargo Dependencies With Fallback
if: {% raw %}${{ inputs.cargo-cache-key && inputs.cargo-cache-fallback-key }}{% endraw %}
uses: actions/cache@v4
with:
path: |
~/.cargo/bin/
~/.cargo/registry/index/
~/.cargo/registry/cache/
~/.cargo/git/db/
target/

See how the target/ path is in both cache jobs?

I wonder if we need to replace that with ./target for it to be more robust? Or maybe add both?

It would be nicer if most cache related things could be tackled within the setup job so we can keep the workflows cleaner.

@samuelvanderwaal
Copy link
Contributor Author

Hey Sam! Thanks for raising this. It's strange because I thought this would be taken care of by the cache jobs inside the setup actions.

- name: Cache Cargo Dependencies
if: {% raw %}${{ inputs.cargo-cache-key && !inputs.cargo-cache-fallback-key }}{% endraw %}
uses: actions/cache@v4
with:
path: |
~/.cargo/bin/
~/.cargo/registry/index/
~/.cargo/registry/cache/
~/.cargo/git/db/
target/
key: {% raw %}${{ runner.os }}-${{ inputs.cargo-cache-key }}-${{ hashFiles('**/Cargo.lock') }}{% endraw %}
restore-keys: {% raw %}${{ runner.os }}-${{ inputs.cargo-cache-key }}{% endraw %}
- name: Cache Cargo Dependencies With Fallback
if: {% raw %}${{ inputs.cargo-cache-key && inputs.cargo-cache-fallback-key }}{% endraw %}
uses: actions/cache@v4
with:
path: |
~/.cargo/bin/
~/.cargo/registry/index/
~/.cargo/registry/cache/
~/.cargo/git/db/
target/

See how the target/ path is in both cache jobs?

I wonder if we need to replace that with ./target for it to be more robust? Or maybe add both?

It would be nicer if most cache related things could be tackled within the setup job so we can keep the workflows cleaner.

Let me try updating my repo with ./target to see if that fixes the issue.

@samuelvanderwaal
Copy link
Contributor Author

samuelvanderwaal commented Nov 14, 2024

Still get this error:

[ SUCCESS ] The expected Solana version 1.16.23 is installed.
Starting local validator with 2 custom programs and 1 external account...
  ⠋ Waiting for local validator to stabilize...
  ⠙ Waiting for local validator to stabilize...
  ⠹ Waiting for local validator to stabilize...
  ⠸ Waiting for local validator to stabilize...
  ⠼ Waiting for local validator to stabilize...
  ⠴ Waiting for local validator to stabilize...
  ⠦ Waiting for local validator to stabilize...
  ⠧ Waiting for local validator to stabilize...
  ⠇ Waiting for local validator to stabilize...

Notice! No wallet available. `solana airdrop` localnet SOL after creating one

Ledger location: test-ledger
Log: test-ledger/validator.log
Initializing...
Error: add_accounts_from_json_files failed: Unable to locate /home/runner/work/my-project/my-project/target/deploy/<account>.json

Could not start local validator.

I think the setup environment caching happens before programs build so isn't capturing the downloaded accounts, right?

@lorisleiva
Copy link
Member

Oh wait sorry I'm being stupid. This is because the cargo-cache-key: cargo-programs is not set when testing the JS client, since all it needs is the .so files and (now as you pointed out) the dumped files. I think this PR makes total sense then.

As a little nit, I wonder if it would be cleaner to combine this with the previous step that save/restore the so files. We could call it like save/restore client artifacts or something. Wdyt?

@samuelvanderwaal
Copy link
Contributor Author

Oh wait sorry I'm being stupid. This is because the cargo-cache-key: cargo-programs is not set when testing the JS client, since all it needs is the .so files and (now as you pointed out) the dumped files. I think this PR makes total sense then.

As a little nit, I wonder if it would be cleaner to combine this with the previous step that save/restore the so files. We could call it like save/restore client artifacts or something. Wdyt?

Yes, I think that makes sense!

@samuelvanderwaal
Copy link
Contributor Author

One thing to note is this also caches the local keypair which is also a *.json file in target/deploy. I think this is what we want as it can then be used in the local validator for tests, but just thought I'd flag it in case you see an issue with that.

Copy link
Member

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

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

Thank you!

@lorisleiva lorisleiva merged commit b6115d0 into solana-program:main Nov 15, 2024
6 checks passed
@github-actions github-actions bot mentioned this pull request Nov 15, 2024
@samuelvanderwaal samuelvanderwaal deleted the fix/cache-restore-external-accounts branch November 15, 2024 15:01
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.

2 participants