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

Include image version in computed cache key #75

Merged
merged 1 commit into from
Feb 8, 2021

Conversation

orudge
Copy link
Contributor

@orudge orudge commented Feb 8, 2021

This hopefully offers a fix for #69, which we have been encountering. The latest update fixed the issue not being reported, but we still had issues because the version that had been cached was too old and was throwing compile errors.

ImageOS is also exported as an environment variable (for all of Windows, Linux, macOS on GitHub Actions) though I haven't used it here. It may be worth incorporating it too, or replacing the use of process.platform with process.env.ImageOS.

(It may be the if statement is also redundant, and we can always assume these variables are present.)

@@ -116,6 +116,11 @@ export class Utils {

key += "-args=" + Utils.hashCode(core.getInput(runvcpkglib.vcpkgArguments));
key += "-os=" + Utils.hashCode(process.platform);

if (process.env.ImageVersion) {
key += "-imageVer=" + Utils.hashCode(process.env.ImageVersion);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should IMAGE_VERSION be used instead, given this context?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I'd missed that - yes, I suspect so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've switched this to use IMAGE_VERSION, and have also added a preference for IMAGE_OS instead of process.platform in the "os" key.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, no, I think those variables are only used when configuring the VMs, but are not accessible at “runtime”. ImageVersion / ImageOs seem to be the intended variables to use. I’ll fix this again.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only side-effect of these changes is that the existing cached artifacts are all invalidated. This means I will publish a run-vcpkg@v7, i.e. bumpoing the major version, as this going to force existing workflows to regenerate all cached artifacts.

@orudge orudge force-pushed the include-image-version branch from 9e67bba to 0264890 Compare February 8, 2021 16:43
@orudge orudge force-pushed the include-image-version branch from 0264890 to d547a12 Compare February 8, 2021 18:50
@lukka
Copy link
Owner

lukka commented Feb 8, 2021

@orudge Looks good to me, thanks!
As a reference, this issue track the changes done to add ImageOS and its version: actions/runner-images#345

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.

3 participants