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.

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

tool-cache: Use --force-local for tar on Windows #165

Closed
wants to merge 1 commit into from
Closed

tool-cache: Use --force-local for tar on Windows #165

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
8 changes: 7 additions & 1 deletion packages/tool-cache/src/tool-cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,13 @@ export async function extractTar(

dest = dest || (await _createExtractFolder(dest))
const tarPath: string = await io.which('tar', true)
await exec(`"${tarPath}"`, [flags, '-C', dest, '-f', file])
const args: string[] = [flags]
if (IS_WINDOWS) {
// On Windows, absolute path use a ':' (e.g. C:\foo), which is incorrectly recognized as a remote path by tar unless this flag is passed
args.push('--force-local')
}
args.push('-C', dest, '-f', file)
Copy link
Contributor

@ericsciple ericsciple Dec 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

According to C:\windows\system32\tar.exe --help the option -C is only for create, it is not a common option.

Also @joshmgross informed me bsd tar does not support --force-local. And C:\windows\system32\tar.exe is bsd tar.

Instead of -C I think dest should be the working directory (exec option).

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

ExecOptions.cwd

Copy link

Choose a reason for hiding this comment

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

@ericsciple, while using option cwd works, I am not sure about --force-local. It seems to be required. See:

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see. Because of the other file path 🤦

The problem is the hosted machines prepend the gnu tools to the PATH, rather than append to the PATH.

Self hosted runners would more commonly have %windir%\System32\tar.exe (bsd tar) in the PATH.

And bsd tar works correctly. It also doesn't require forward slashes.

I think the correct solution is: on Windows don't which tar.

Instead do:

`${process.env['windir']}\\System32\\tar.exe`

If tar doesn't exist at that location, fail.

Thoughts?

Copy link

@eine eine Dec 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@ericsciple, thanks for the details. If that is the case, I think that more effort is required than just fixing if (IS_WINDOWS) in this repo:

  • The toolkit should provide a tar command that would hide the inconsistencies in the implementation of the runners from the user. Not using which has the side effect that the arguments might need to be explicitly different for windows-latest, ubuntu-latest and macos-latest.
  • Your comment about gnu tools being prepended to the default (bsd flavour), should be added to the docs as an important warning.

Then, it will be up to action developers to use the command or to handle it with custom logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont like the solution i proposed anymore. @thboop good point that it doesnt cover older windows which the runner supports.

await exec(`"${tarPath}"`, args)

return dest
}
Expand Down