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: add arch to cache key #896

Merged
merged 2 commits into from
Aug 7, 2024
Merged

fix: add arch to cache key #896

merged 2 commits into from
Aug 7, 2024

Conversation

Zxilly
Copy link
Contributor

@Zxilly Zxilly commented Jun 18, 2024

Description:

GitHub has added the arm64 runner, so arch should be used as part of the cache key. Otherwise, if there are macos-13 macos-14 running at the same time, the late executor will get the wrong cache.

This problem is particularly acute for pipenv and poetry, for which action also caches the contents of venv. This prevents any libraries containing native binary dependencies from working properly.

For real-world error, see https://github.com/Zxilly/go-size-analyzer/actions/runs/9564444574/job/26365211564

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@Zxilly Zxilly requested a review from a team as a code owner June 18, 2024 12:31
@Zxilly
Copy link
Contributor Author

Zxilly commented Jun 19, 2024

@actions/setup-actions-team Can anyone review this?

@Zxilly
Copy link
Contributor Author

Zxilly commented Jul 13, 2024

@actions/setup-actions-team I'm wondering if anyone is still maintaining this project, between the fact that GitHub has started to offer the Ubuntu arm64 runner, this problem is getting worse.

@Zxilly
Copy link
Contributor Author

Zxilly commented Jul 13, 2024

@HarithaVattikuti @priya-kinthali Can you take a look on this?

@aparnajyothi-y
Copy link
Contributor

Hello @Zxilly, Thank you for this PR, we are looking into it and we will get back to you once we have some feedback on this :)

@aparnajyothi-y
Copy link
Contributor

Hello @Zxilly, We have reviewed and tested the changes from our end for Including arch in the cache key has made the run successful. However, we are unable to see arch in the cache key with all the changes of this PR. Please find the screenshots for reference. Please have a look at this PR and update to have this to be approved and merged.
Screenshot 2024-07-24 at 5 38 53 PM
Screenshot 2024-07-24 at 5 42 15 PM

@Zxilly
Copy link
Contributor Author

Zxilly commented Jul 25, 2024

@aparnajyothi-y Apparently the cache in the image is an npm cache created by setup-node, does it have anything to do with this PR? This repo is setup-python

@Zxilly
Copy link
Contributor Author

Zxilly commented Jul 25, 2024

I think setup-node has a similar PR actions/setup-node#843 and got no review for a long time.

@aparnajyothi-y
Copy link
Contributor

Hello @Zxilly, Please find attached the setup-python cache key screenshots for reference where the changes of Including arch in the cache key has made the run successful. However, we are unable to see arch in the cache key with all the changes of this PR.
Please have a look at this PR and update to have this to be approved and merged.

image image

@Zxilly
Copy link
Contributor Author

Zxilly commented Jul 25, 2024

You use the https://github.com/aparnajyothi-y/setup-python/commits/PR%23896-test/ workflow.

The branch you created only contains changes to the ts file, but GitHub Actions executes the js file, and you should compile the ts file to js to allow it to work properly.

@Zxilly
Copy link
Contributor Author

Zxilly commented Jul 25, 2024

If you want to test this PR, you should directly use Zxilly/setup-python@main

@aparnajyothi-y
Copy link
Contributor

Hello @Zxilly, Could you please confirm that the code has passed the checks executed by npm run format-check and npm test? We are currently unable to run / view the checks on this PR and need confirmation to proceed with merging this PR. Thank you.

@Zxilly
Copy link
Contributor Author

Zxilly commented Aug 6, 2024

image
image

All passed.

@Zxilly
Copy link
Contributor Author

Zxilly commented Aug 6, 2024

I'm curious, why don't you execute the tests yourself? It's unimaginable that such a simple PR took almost two months.

@HarithaVattikuti HarithaVattikuti merged commit 80b49d3 into actions:main Aug 7, 2024
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.

None yet

7 participants