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 cached path #843

Merged
merged 5 commits into from
Oct 24, 2024
Merged

fix: add arch to cached path #843

merged 5 commits into from
Oct 24, 2024

Conversation

pengx17
Copy link
Contributor

@pengx17 pengx17 commented Aug 29, 2023

Description:
The existing node cache key does not take architecture into consideration. However during npm/yarn/etc installing the dependencies may generate different binaries based on the current architecture. Thus if the workflow has matrix that spawn different jobs on different architectures for the same platform may consume the same cache, which might be wrong.

This PR adds the arch to the cached key.

Related issue:
Add link to the related issue.

Check list:

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

@vincentbasp
Copy link

Hello,
This pull request fixes the issue described in issue by adding the architecture to the cache key. I tested this change on my repository and it works fine.
Do you know when this pr will be merged ?
Thanks to @pengx17 for his work

@aparnajyothi-y
Copy link
Contributor

Hello @pengx17, Could you please update the line os.platform() with process.env.RUNNER_OS to maintain the existing value of process.env.RUNNER_OS for better consistency and clarity with the runner name.

@aparnajyothi-y
Copy link
Contributor

Hello @pengx17, a gentle follow up to update the line os.platform() with process.env.RUNNER_OS to maintain the existing value of process.env.RUNNER_OS for better consistency and clarity with the runner name.

@pengx17
Copy link
Contributor Author

pengx17 commented Sep 24, 2024

Hello @pengx17, a gentle follow up to update the line os.platform() with process.env.RUNNER_OS to maintain the existing value of process.env.RUNNER_OS for better consistency and clarity with the runner name.

Sorry, just noticed the message. Done

@HarithaVattikuti HarithaVattikuti merged commit 39370e3 into actions:main Oct 24, 2024
160 checks passed
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.

6 participants