-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
tool-cache: Use --force-local for tar on Windows #165
Conversation
Thanks! Can you include a workflow with a repro? I want to validate with and without your changes. |
Hi! I'm not sure what you're asking for exactly. My understanding is that a As far as a test case could go, I think this should do the trick: const myArchive = await tc.downloadTool("http://host.com/path/to/tarball.tgz");
const archiveExtractedFolder = await tc.extractTar(node12Path, "."); |
Note that when testing this patch you might also get hit by #180 which is yet another bug on Windows 😄 |
thanks for this! it (and #180) were necessary for me too. |
// 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 usingwhich
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment
Ppl please stop emailing me...have no idea what this about
…On Fri, Dec 6, 2019, 9:28 PM eric sciple ***@***.***> wrote:
***@***.**** requested changes on this pull request.
see comment
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#165?email_source=notifications&email_token=AH6VJYAZ4SC7WT3ZK7DYD53QXMJ6BA5CNFSM4I3XMMD2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCOKRA6A#pullrequestreview-328536184>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AH6VJYHPQM4EGI2YMDLI363QXMJ6BANCNFSM4I3XMMDQ>
.
|
@ericsciple, the scenario is numworks/setup-msys2: See also numworks/setup-msys2#23. The big picture is that the As commented in actions/starter-workflows#95, this is, overall, the best but limited workaround we can use to overcome the fact that GHA are not MSYS2-friendly at all. Should GitHub provide built-in
Unfortunately, pre-built versions of MSYS2 are distributed as As commented in https://github.community/t5/GitHub-Actions/Windows-MSYS2-Ruby/m-p/33946/highlight/true#M1727, before using setup-msys2, I was using chocolatey to install MSYS2. However, it takes 5min to do so. setup-msys2 needs around 30s.
I don't think this is an issue, since maintainers of self-hosted runners can (should) install MSYS2 as part of the base image. Hence, step setup-msys2 might just be skipped. |
@eine thanks, that helps. I checked |
@ericsciple, precisely, see actions/runner-images#30 and actions/runner-images#50. |
Fixed by #264 |
The
extractTar
nearly works on Windows, except that it cannot deal with absolute path.Indeed,
C:\foo\bar
is interpreted as a remote path by tar by default.