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 caching by ignoring semver build metadata #60

Draft
wants to merge 5 commits into
base: default
Choose a base branch
from

Conversation

jethrodaniel
Copy link

@jethrodaniel jethrodaniel commented Nov 24, 2023

Fixes #59
Fixes #72
Fixes #75

See comment: #59 (comment)

Seems to fix the caching issue for me.

@goto-bus-stop
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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

Any updates on this?

@squeek502
Copy link

@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:

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.

older Zig version installed instead of 0.12.0 because of caching Caching not working Caching Failure
4 participants