setup-node icon indicating copy to clipboard operation
setup-node copied to clipboard

Consider including `architecture` in cache keys

Open silverwind opened this issue 1 year ago • 6 comments

Description: The cache keys are currently node-cache-Linux-npm-<hash> which does not seem to include the CPU architecture so if npm dependencies include native binaries and the same cache is restored on a different architecture than what it was built on, these binaries would fail to run.

Action version: 4

Platform: All

Runner type: All

Tools version: All

Repro steps:

  • Build a cache on a x86 runner
  • Restore cache on a ARM runner

Expected behavior: Cache key should include architecture

Actual behavior: Cache key does not include architecture

silverwind avatar Apr 19 '24 09:04 silverwind

Hello @silverwind Thank you for creating this issue. We will investigate it and get back to you as soon as we have some feedback.

HarithaVattikuti avatar Apr 19 '24 22:04 HarithaVattikuti

Can the same be done for Node version as well? If you set up a matrix that runs the same workflow on both e.g. Node 16 and 18, and you have packages like jsdom or canvas which get compiled for a particular Node version like https://github.com/Automattic/node-canvas/issues/1410 , wouldn't the same cache be downloaded for both Node 16 and Node 18, thus causing the linked error?

Let me know if I should make a separate issue

sethidden avatar Apr 24 '24 12:04 sethidden

Yes, different node major version have different ABI versions, so I would recommend including node major version in the cache key. Minor/patch versions should be strictly ABI compatible. Also see https://nodejs.org/en/learn/modules/abi-stability.

silverwind avatar Apr 24 '24 13:04 silverwind

Pretty sure this would resolve a problem with rollup where it uses a binary for a specific architecture. The binary for the first build is cached and then there are failures on subsequent builds if the cached arch. doesn't match the current arch. The workaround is to add the different architecture packages as optional dependencies.

Error: Cannot find module @rollup/rollup-linux-x64-gnu

rlmcneary2 avatar May 21 '24 14:05 rlmcneary2

Hi @silverwind, Thanks for your valuable suggestion. After analyzing the issue, It seems to be more of a potential feature request rather than a bug. The idea of incorporating the architecture in cache keys could be a significant step in managing npm dependencies with native binaries on various architectures. We've taken your suggestion into consideration for possible future enhancements .

gowridurgad avatar Jun 13 '24 11:06 gowridurgad

FYI, ARM runners are already in "public" beta: https://github.blog/changelog/2024-06-03-actions-arm-based-linux-and-windows-runners-are-now-in-public-beta/

silverwind avatar Jun 13 '24 12:06 silverwind