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

use windows tar, not git bash one #206

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

IanButterworth
Copy link
Member

Fixes #205

@IanButterworth IanButterworth requested a review from a team as a code owner January 15, 2024 18:25
Copy link
Contributor

@omus omus left a comment

Choose a reason for hiding this comment

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

Can we add a test for this?

@IanButterworth
Copy link
Member Author

One where we delete/rename the system tar?

src/installer.ts Outdated Show resolved Hide resolved
IanButterworth and others added 2 commits January 15, 2024 18:42
Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
@DilumAluthge
Copy link
Member

Users may have custom self-hosted Windows runners. In those cases, currently users can specify which tar is used by manually adding it to the front of the PATH before setup-julia is run.

However, after this PR, the user cannot control which tar is used. Could we add an escape hatch? Maybe an action input that allows the user to specify the desired tar? And then if the user doesn't specify that action input, we fall back to this hardcoded "$env:WINDIR/System32/tar"?

@cderv
Copy link

cderv commented Jan 16, 2024

In those cases, currently users can specify which tar is used by manually adding it to the front of the PATH before setup-julia is run.

This could definitely be a solution to avoid the issue from #305 - though I still think defaulting to tar.exe from Windows if it exists would be better.

Could we add an escape hatch? Maybe an action input that allows the user to specify the desired tar? And then if the user doesn't specify that action input, we fall back to this hardcoded "$env:WINDIR/System32/tar"?

Selecting the right tar could be a good escape hatch - it could also be through an environment variable maybe, which is less user-facing maybe.

Are custom self-hosted Windows runners using a different tar.exe?

For the new context, I found the real why about #205 (detailed in #207 (comment))

The real problem was that another action was changing the PATH internally tweaking $GITHUB_PATH. This led to the wrong tar being used - by wrong I mean a tar binary that does not handle the untar from D: to C:

Currently, julia-actions/setup-julia use any tar from PATH, including any that could be incompatible with the expected command

yield exec.exec('powershell', ['-Command', `tar xf ${juliaDownloadPath} --strip-components=1 -C ${dest}`]);

  • juliaDownloadPath will be in D: as it is based on RUNNER_TEMP by default through tc.downloadTool()

    setup-julia/dist/index.js

    Lines 234 to 236 in a1561e9

    const juliaDownloadPath = yield retry((bail) => __awaiter(this, void 0, void 0, function* () {
    return yield tc.downloadTool(downloadURL);
    }), {

  • dest will be a folder in the hostedtoolcache folder on C:

    setup-julia/dist/index.js

    Lines 429 to 439 in a1561e9

    core.debug(`could not find Julia ${arch}/${version} in cache`);
    // https://github.com/julia-actions/setup-julia/pull/196
    // we want julia to be installed with unmodified file mtimes
    // but `tc.cacheDir` uses `cp` internally which destroys mtime
    // and `tc` provides no API to get the tool directory alone
    // so hack it by installing a empty directory then use the path it returns
    // and extract the archives directly to that location
    const emptyDir = fs.mkdtempSync('empty');
    juliaPath = yield tc.cacheDir(emptyDir, 'julia', version, arch);
    yield installer.installJulia(juliaPath, versionInfo, version, arch);
    core.debug(`added Julia to cache: ${juliaPath}`);

So to be safer there could be two solutions at least :

  • ensuring either the right tar (like in this PR + a potential option) or
  • ensuring the same drive is used calling tc.downloadTool() with a non default dest.

Thanks a lot.

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.

[BUG] Using tar on Windows should call $env:WINDIR/System32/tar and not the Git bash provided /usr/bin/tar
5 participants