fix extractTar on Windows #248
Conversation
| // `--force-local` due to the `:` in Windows paths. This presents an incompatibility because | ||
| // BSD tar does not support the flag `--force-local` (invalid flag, fails the process). | ||
| tarPath = path.join(process.env['windir'] as string, 'System32', 'tar.exe') | ||
| fs.statSync(tarPath) |
There was a problem hiding this comment.
What's the purpose of fs.StatSync(tarPath)? To throw if tar doesn't exist?
There was a problem hiding this comment.
should probably use existsSync instead of this mechanism, if that's the case...
There was a problem hiding this comment.
yes, i'll imrpove the error message
There was a problem hiding this comment.
actually tool runner provides a good error message so i'll just remove this line
|
Did you verify that BSD tar on Windows doesn't have issues with |
| // Windows-style paths. Whereas GNU tar requires forward slashes and also requires the flag | ||
| // `--force-local` due to the `:` in Windows paths. This presents an incompatibility because | ||
| // BSD tar does not support the flag `--force-local` (invalid flag, fails the process). | ||
| tarPath = path.join(process.env['windir'] as string, 'System32', 'tar.exe') |
There was a problem hiding this comment.
Tar seems to have been added as a part of the initiative to bring more Unix tools to windows in Windows 10. It looks liked our only hosted version of windows is now 2019, and it exists on that machine for hosted builds
However, for self hosted runners do we know if this exists on all the platforms that can run .net core 3.0? Those go back all the way to Windows Server 2012. https://github.com/dotnet/core/blob/master/release-notes/3.0/3.0-supported-os.md
For 7zip, we ship a copy of the binary and and use it if needed, we may need to look into a similar solution here.
toolkit/packages/tool-cache/src/tool-cache.ts
Line 153 in d823f75
There was a problem hiding this comment.
Should we attempt to detect if the path doesn't exist so we can log a better message in this case if not all windows versions will have it installed. We should let a user know explicitly that we are using the tar version build into windows, your version of windows does not have it please upgrade " or provide a user workaround of some sort and bake that into the error message.
There was a problem hiding this comment.
Looks like maybe using existsSync and handling the false case would be the best way to do this. I wouldn't attempt to ship a binary in this case... maybe just fail and explain why.
There was a problem hiding this comment.
+1 regarding the error message, at a glance it looked good to me but looking again we can make it a little bit more clear by try/catch/throw a better error.
one problem with the 7z approach is this function is literally designed as a wrapper over tar (see the flags parameter`)
There was a problem hiding this comment.
Agreed on using 7zip isn't the correct solution here. I don't really like shipping an exe of tar with the package like we do with 7zip either.
I'm happy with better error message (this may also affect users using container actions because iirc we support windows containers if the host system is linux)
There was a problem hiding this comment.
actually tool runner gives a good error already so i'll remove the statSync line
There was a problem hiding this comment.
looking into 7z. note we dont package the full 7z (over 1mb) just the decoder exe (40kb)
There was a problem hiding this comment.
i talked w/ josh and basically the cache action needs the support
There was a problem hiding this comment.
so will need broad os support.
i still think the current proposal is incrementally better
There was a problem hiding this comment.
Re: Hosted Runners vs Self-Hosted Runners
The current behavior is that we use GNU specific tar option on windows (specifically --force-local) since the GNU tar is prepended to the PATH.
This works for hosted runners, but self-hosted runners most likely do not have GNU prepended, so they will either fail with tar not being found or BSD tar not recognizing --force-local), see actions/cache#91
With this solution, at least self-hosted runners that have tar are supported.
Also worth noting the cache action does not consume this package, it just has the same problems with tar.
| // `--force-local` due to the `:` in Windows paths. This presents an incompatibility because | ||
| // BSD tar does not support the flag `--force-local` (invalid flag, fails the process). | ||
| tarPath = path.join(process.env['windir'] as string, 'System32', 'tar.exe') | ||
| fs.statSync(tarPath) |
|
@thboop @hross @joshmgross thoughts? ok so 7zdec doesnt work for .tar.gz and .tar.xz other solutions on Windows that come to mind are:
|
|
honestly at this point i think i'm going to close this PR and open an ADR. I dont feel good anymore about the half solution i proposed with this PR. I would rather come up with a proposal that allows us to fix it once and for all, and never revisit the problem again. |
Fixes #194