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

fix caching by ignoring semver build metadata

Open jethrodaniel opened this issue 2 years ago • 5 comments

Fixes https://github.com/goto-bus-stop/setup-zig/issues/59 Fixes #72 Fixes #75

See comment: https://github.com/goto-bus-stop/setup-zig/issues/59#issuecomment-1825139671

Seems to fix the caching issue for me.

jethrodaniel avatar Nov 24 '23 04:11 jethrodaniel

Thanks for looking into this! Do I understand correctly that the issue is because the extracted path is different from the archive file name?

I think the commit hash should be part of the cache key, else changing the version in your workflow will reuse a different version from cache.

goto-bus-stop avatar Nov 28 '23 12:11 goto-bus-stop

Thanks for looking into this! Do I understand correctly that the issue is because the extracted path is different from the archive file name?

Yep - seems the final path returned by downloadZig omits the build metadata hash

  const cachePath = await toolCache.cacheDir(binPath, TOOL_NAME, useVersion)

  if (useCache) {
    actions.info(`adding zig ${useVersion} at ${cachePath} to local cache ${cacheKey}`)
    await cache.saveCache([cachePath], cacheKey)
  }

  return cachePath

Which Is what I noticed in the issue.

Looking at @actions/tool-cache, I noticed that the cacheDir function uses semver.clean:

/**
 * Caches a directory and installs it into the tool cacheDir
 *
 * @param sourceDir    the directory to cache into tools
 * @param tool          tool name
 * @param version       version of the tool.  semver format
 * @param arch          architecture of the tool.  Optional.  Defaults to machine architecture
 */
export async function cacheDir(
  sourceDir: string,
  tool: string,
  version: string,
  arch?: string
): Promise<string> {
  version = semver.clean(version) || version

And semver.clean strips out the build metadata, since semver says it doesn't matter for versioning purposes.

Build metadata MUST be ignored when determining version precedence. Thus two versions that differ only in the build metadata, have the same precedence. Examples: 1.0.0-alpha+001, 1.0.0+20130313144700, 1.0.0-beta+exp.sha.5114f85, 1.0.0+21AF26D3----117B344092BD.

Adding this to test.js results in

  assert.equal(
    semver.clean('0.12.0-dev.1150+3c22cecee'),
    '0.12.0-dev.1150+3c22cecee'
  )
$ standard && node test
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ '0.12.0-dev.1150'
- '0.12.0-dev.1150+3c22cecee'

So basically, it's @actions/tool-cache that's forcing this.

I think the commit hash should be part of the cache key, else changing the version in your workflow will reuse a different version from cache.

Per semver, build metadata isn't actually valuable version information.

But, since zig's versioning uses a nicely defined patch version with an ever-increasing "build" number (e.g, dev.1150), we shouldn't have any issues with the cached version of zig not matching up (unless I'm missing something here).

Also weird that semver doesn't actually seem to have a test for this: https://github.com/npm/node-semver/blob/main/test/functions/clean.js


Note

PR is still a draft, since this is mostly for explanation purposes, but I'd be happy to clean it up and move it forward after consenus here.

Also, once again, thanks for this repo!

Def easier and cleaner than having folks wget/curl zig themselves and using actions/cache directly (which is what I did before investigating this 🐛 )

jethrodaniel avatar Nov 29 '23 18:11 jethrodaniel

Thanks very much for that research. Sorry it is taking so long to follow up! I'll hopefully find some time to wrap up the open PRs on this repo this week 😅

goto-bus-stop avatar Jan 29 '24 09:01 goto-bus-stop