Skip to content

fix: add arch to cached path#843

Merged
HarithaVattikuti merged 5 commits into
actions:mainfrom
pengx17:main
Oct 24, 2024
Merged

fix: add arch to cached path#843
HarithaVattikuti merged 5 commits into
actions:mainfrom
pengx17:main

Conversation

@pengx17

@pengx17 pengx17 commented Aug 29, 2023

Copy link
Copy Markdown
Contributor

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.

Comment thread src/cache-restore.ts Outdated
@vincentbasp

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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

pengx17 commented Sep 24, 2024

Copy link
Copy Markdown
Contributor Author

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

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