Skip to content

Avoid version corruption by passing loose option to semver.clean in the tool-cache package#698

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.

Sign up for GitHub

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

Closed
smorimoto wants to merge 1 commit into from
Closed

Avoid version corruption by passing loose option to semver.clean in the tool-cache package #698

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions packages/tool-cache/src/tool-cache.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -381,7 +381,7 @@ export async function cacheDir(
version: string,
arch?: string
): Promise<string> {
version = semver.clean(version) || version
version = semver.clean(version, {loose: true}) || version
arch = arch || os.arch()
core.debug(`Caching tool ${tool} ${version} ${arch}`)

Expand DownExpand Up@@ -422,7 +422,7 @@ export async function cacheFile(
version: string,
arch?: string
): Promise<string> {
version = semver.clean(version) || version
version = semver.clean(version, {loose: true}) || version
arch = arch || os.arch()
core.debug(`Caching tool ${tool} ${version} ${arch}`)

Expand DownExpand Up@@ -478,7 +478,7 @@ export function find(
// check for the explicit version in the cache
let toolPath = ''
if (versionSpec) {
versionSpec = semver.clean(versionSpec) || ''
versionSpec = semver.clean(versionSpec, {loose: true}) || ''

@thboopthboopFeb 5, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This really only works for explicit versions, for example:

 await tc.cacheFile(downloadPath, 'foo', 'foo', '1.1.0-beta')
const toolPath: string = tc.find('foo', '1.1.0-beta')

Ideally, we would support beta/alpha versions, and let users opt in or out when using find so you could do something like

 await tc.cacheFile(downloadPath, 'foo', 'foo', '1.1.0-beta')
const toolPath: string = tc.find('foo', '1.1.x' {allowPreview: true})

I'll go ahead and file an issue to see if we can get started on supporting non-semvar versions and preview versions, but I don't want to partially support beta versions at this time.

@smorimotosmorimotoFeb 6, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

As I've said before, the problem here is that not every programming language, ecosystem, software, etc... in the world is fully compliant with Semver (Here is a good thread to explain that: https://twitter.com/_craigfe/status/1357277471364829184), and most software in the world does not strictly adhere to Semver. (Most are just acting as if they are.) In addition, passing non-explicit versions is a wrong thing. Do we really need to care? The reason this is wrong is that unintentional cache calls are often confusing and often take a long time to debug because no one thinks they are the cause. In other words, we should not manage our cache loosely unless we are sure that it's really safe. This is the same thing as why we recommend caching yarn or npm cache directory, rather than node_modules itself.

@smorimotosmorimotoFeb 12, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Still waiting for your response.

@smorimotosmorimotoFeb 21, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ping

@thboopthboopMar 11, 2021•
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

in the world is fully compliant with Semver ... and most software in the world does not strictly adhere to Semver.

I agree 100% , its why I filed #709, we need a better way to support non-semver items into the tool cache.

The reason this is wrong is that unintentional cache calls are often confusing and often take a long time to debug because no one thinks they are the cause. In other words

I don't think user's are calling the cache unintentionally, it's a pretty explicit interface to call the cache.

This is the same thing as why we recommend caching yarn or npm cache directory, rather than node_modules itself.

The tool cache serves a different purpose from the cache action. It is used to speed up workflow execution by having dependencies available locally, and is available runner wide rather than job wide. Since having every version of something like node installed locally may be unfeasible, semver allows users to state a range of versions that work for them.

For example, take a look at our cached tools, notice only three versions of node.
If users had to pin to exact versions, they would likely never hit the cache on the tool-cache. Furthermore, caching every version may quickly cause runners to run out of disk space, as these cache's are persisted and runners can be assigned to orgs, rather than individual repos. Typically our hosted runners just cache the latest major/minor versions of major dependencies actions users may use.

While this pr works for self hosted runners who need to do exactly this flow, but by merging it we commit to supporting this interface in future versions. The current interface supports semver because it makes sense for the use case of the tool-cache, while we should support other types of versioning, any additions we add should also support semver to be consistent with how this feature currently works.

@smorimotosmorimotoMar 11, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I agree 100% , its why I filed #709, we need a better way to support non-semver items into the tool cache.

I'm glad to hear that.

I don't think user's are calling the cache unintentionally, it's a pretty explicit interface to call the cache.

For example, imagine that you have configured a tool to compile to that location that is not semver-compliant and is not relocatable (such that paths will be hardcoded). Let's suppose it's 1.0.0+musl. However, it's stored as 1.0.0 and the directory is handed over unintentionally. When this happens, it's a big mistake. When it's called, the tool tries to look at the 1.0.0+musl directory, but it actually stores 1.0.0, and it will fail.

While this pr works for self hosted runners who need to do exactly this flow, but by merging it we commit to supporting this interface in future versions. The current interface supports semver because it makes sense for the use case of the tool-cache, while we should support other types of versioning, any additions we add should also support semver to be consistent with how this feature currently works.

That's a good point, but don't the toolkit adhere semver? Is it not enough to bump a major version? I think it's better to unify the handling of all functions with no extra options.

@smorimotosmorimotoMar 12, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I forgot to write in the comment above; I think it would be better to stop even using semver.clean.

@thboopthboopJun 1, 2021•
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@smorimoto , Could you describe the scenario more, I don't understand how this PR addresses it.

In semver, there are two formats you can expect outside of the general 2.0.0

  • build version 2.0.0+{build metadata}
  • prerelease 2.0.0-{prerelease identifier}

According to semver, the build metadata must be ignored, so your example 1.0.0+musl needs to be treated the same as 1.0.0.

If you had a done a prerelease identifier 1.0.0-musl. The tool cache already handles that. It creates a folder named 1.0.0-musl
See the below code:

 await tc.cacheFile(downloadPath, 'foo', 'foo', '1.1.0-musl')
const toolPath: string = tc.find('foo', '1.1.0-musl') // this works
// the below would fail because it tries to find 1.1.0, not the prerelease version
const toolPath: string = tc.find('foo', '1.1.0')

However, it's stored as 1.0.0

I want to be very clear, there is no version corruption for prerelease identifiers. The corruption you are seeing for build identifiers it not actually corruption, that is how semver is intended to work.

You can also already store and access prerelease versions, and they don't result in version corruption.

All the clean option does it help scenarios where the input is invalid semver, like

 await tc.cacheFile(downloadPath, 'foo', 'foo', '1.1.0musl')
await tc.cacheFile(downloadPath, 'foo', 'foo', '1.1.0musl')

I'm definitely willing to take enhancements for this feature, particularly around how we can expose includePrerelease from the semver library to the tool cache and allow users to do semver matching on prerelease tags, which may help your use case, but I don't think this PR solves it.

That being said, I appreciate your diligence in bringing these items to our attention, I'm planning to make it through the rest of your prs this week, thanks for the contributions!

I'm going to close out this pr until its ready for review again.

const cachePath = path.join(
_getCacheDirectory(),
toolName,
Expand DownExpand Up@@ -626,7 +626,7 @@ async function _createToolPath(
const folderPath = path.join(
_getCacheDirectory(),
tool,
semver.clean(version) || version,
semver.clean(version, {loose: true}) || version,
arch || ''
)
core.debug(`destination ${folderPath}`)
Expand All@@ -641,7 +641,7 @@ function _completeToolPath(tool: string, version: string, arch?: string): void {
const folderPath = path.join(
_getCacheDirectory(),
tool,
semver.clean(version) || version,
semver.clean(version, {loose: true}) || version,
arch || ''
)
const markerPath = `${folderPath}.complete`
Expand All@@ -650,7 +650,7 @@ function _completeToolPath(tool: string, version: string, arch?: string): void {
}

function _isExplicitVersion(versionSpec: string): boolean {
const c = semver.clean(versionSpec) || ''
const c = semver.clean(versionSpec, {loose: true}) || ''
core.debug(`isExplicit: ${c}`)

const valid = semver.valid(c) != null
Expand Down