fix caching by ignoring semver build metadata

jethrodaniel commented 9 months ago

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.

goto-bus-stop commented 8 months ago

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.

jethrodaniel commented 8 months ago

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 🐛 )

goto-bus-stop commented 6 months ago

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 😅

heysokam commented 1 month ago

Any updates on this?

squeek502 commented 1 month ago

@heysokam you might be interested in https://github.com/marketplace/actions/setup-zig-compiler which was created by a Zig core team member

Some links that explain the differences:

goto-bus-stop / setup-zig

fix caching by ignoring semver build metadata #60