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 system tar (not the Git Bash tar) #206

Merged
merged 10 commits into from
Jun 19, 2024
Merged

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.

@DilumAluthge
Copy link
Member

ensuring the same drive is used calling tc.downloadTool() with a non default dest.

I unfortunately don't think that we'll be able to guarantee (in this action) that the same drive is used. I think we need to default to the Windows system tar.

Copy link
Member

@DilumAluthge DilumAluthge left a comment

Choose a reason for hiding this comment

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

I'd prefer #250 instead.

@DilumAluthge
Copy link
Member

Alternatively, we could go with this PR for now, and then add an escape hatch later.

@DilumAluthge DilumAluthge changed the title use windows tar, not git bash one Use windows system tar (not the Git Bash tar) Jun 19, 2024
@DilumAluthge DilumAluthge changed the title Use windows system tar (not the Git Bash tar) Use Windows system tar (not the Git Bash tar) Jun 19, 2024
DilumAluthge
DilumAluthge previously approved these changes Jun 19, 2024
@DilumAluthge
Copy link
Member

Ugh, just because I added a comment, we now need to re-run npm run build and check in the results.

I will do that in a little bit, unless someone beats me to it.

omus
omus previously approved these changes Jun 19, 2024
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.

I'm good to proceed with this

@DilumAluthge DilumAluthge dismissed stale reviews from omus and themself via 90a5ee7 June 19, 2024 20:52
@DilumAluthge DilumAluthge enabled auto-merge (squash) June 19, 2024 20:57
@DilumAluthge
Copy link
Member

macos-11 failures are because of a GitHub brownout. See also #251

@DilumAluthge DilumAluthge merged commit ed4a842 into master Jun 19, 2024
49 of 51 checks passed
@DilumAluthge DilumAluthge deleted the ib/win_tar branch June 19, 2024 20:58
@DilumAluthge
Copy link
Member

I will make a new tag and release.

@DilumAluthge
Copy link
Member

Okay, CI is all green on the release/v2.1.0 branch.

Branch: https://github.com/julia-actions/setup-julia/tree/releases/v2.1.0

Commit: 81d42b5

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