Skip to content

Add retry to installer downloads/installs that had none - #274

Open
djkees wants to merge 2 commits into
fortran-lang:developfrom
djkees:fix/installer-retry-coverage
Open

djkees wants to merge 2 commits into
fortran-lang:developfrom
djkees:fix/installer-retry-coverage

Conversation

@djkees

@djkees djkees commented Sep 16, 2026

Copy link
Copy Markdown

Extends the retry-with-backoff protection already present in some installers (ifx/win32.ts, gfortran/darwin.ts) to a few that were missing it entirely.

  • ifort/win32.ts: wraps the Windows installer download in retry — previously a single, unprotected attempt against the same Intel host that failed in Complete Rewrite: Major upgrade to v2 #245.
  • aocc/debian.ts: wraps the .deb download in retry; also removes a comment that inaccurately claimed built-in resilience.
  • flang/darwin.ts: wraps both its GitHub-releases download and brew install flang in retry.
  • lfortran/debian.ts: swaps a bare conda create call for the condaCreateWithRetry helper already used by the Windows/macOS versions of this same installer.

8 new tests covering retry-success and give-up-after-3-attempts for each.

The 6 failing ifort macOS checks on the fork validation run (djkees#10) were a pre-existing, unrelated Intel DNS outage (getaddrinfo ENOTFOUND registrationcenter-download.intel.com against darwin.ts, which this PR doesn't touch) — not caused by this change.

@djkees

djkees commented Sep 16, 2026

Copy link
Copy Markdown
Author

Side note...I have no more planned edits unless requested until V2 is released and I can help resolve bug reports as they arrive, if any.

@minhqdao

minhqdao commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

I think the last review had many false claims. Updated one:

  1. Dead cleanup guard in downloadToolWithRetry (Flang)​
    src/installers/flang/darwin.ts:264 calls tc.downloadTool(url, destination) where destination is always undefined, so the if (destination) guard at line 268 is unreachable, and the test had to be loosened to expect(..., undefined) to match. To be clear, this does not leak a partial file: @actions/tool-cache@4.0.0 cleans up after itself (downloadToolAttempt wraps the pipeline in try/finally with io.rmRF(dest)). Please either pass an explicit destination — and assert that destination in the test — or drop the optional parameter, so the guard isn't dead code.

  2. Retry-helper duplication — out of scope for this PR
    downloadToolWithRetry now exists in six files: flang/win32.ts, ifx/win32.ts and ifort/win32.ts are byte-identical; gfortran/win32.ts and flang/darwin.ts are identical to each other; aocc/debian.ts is unique (hard-coded User-Agent). brewInstallWithRetry has two copies and they differ — gfortran's passes --skip-post-install, the new flang one does not — so the comment at flang/darwin.ts:288 claiming it "Mirrors ... gfortran/darwin.ts" is inaccurate. I'd rather not expand this PR to resolve the duplication: every path to a shared helper also touches the pre-existing Windows copies (gfortran/win32.ts, ifx/win32.ts, flang/win32.ts), and src/download_installer.ts — the only shared downloader that exists — carries a curl/DNS fallback that has never actually run on Windows (its unit tests mock exec). If you want to take the hoist on, it should be its own PR. What is worth fixing here: the --skip-post-install divergence and that comment.

  3. Regenerate dist after the rebase
    dist/index.js.map cannot merge — its mappings is a single line — so it will be wrong after the rebase. dist/index.js itself auto-merges to a correct build, but the two files have to agree, and hand-repairing a sourcemap isn't worth it. Please re-run the build on top of the rebased branch so both are regenerated. CI's verify-dist job enforces this, but no checks have been reported on this branch yet, so it has never run here.

djkees and others added 2 commits September 29, 2026 15:32
Mirrors the retry-with-backoff patterns already used by ifx/win32.ts and
gfortran/darwin.ts, applied to ifort/win32.ts, aocc/debian.ts, and
flang/darwin.ts's download and brew-install calls, plus lfortran/debian.ts's
conda create (already had a shared retry helper, just wasn't using it).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses review: destination is no longer optional in
downloadToolWithRetry (removes the dead cleanup guard), and
brewInstallWithRetry now passes --skip-post-install like gfortran's does.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@djkees
djkees force-pushed the fix/installer-retry-coverage branch from f22ef24 to 11d4609 Compare September 30, 2026 20:51
@djkees

djkees commented Sep 30, 2026

Copy link
Copy Markdown
Author

Thanks for catching this and correcting the earlier review — appreciated. I'd actually already started hoisting downloadToolWithRetry across all six files before seeing your update, so glad to hear you'd rather keep this PR small; reverted that and went back to just the two files this was originally about.

  • Dropped the optional destination parameter in flang/darwin.ts's downloadToolWithRetry (the dead if (destination) guard is gone) and pass an explicit path at the call site instead. Test now asserts the real destination rather than undefined.
  • Added --skip-post-install to flang/darwin.ts's brewInstallWithRetry to match gfortran/darwin.ts's — the "mirrors" comment is accurate again.
  • Rebuilt dist on top of the rebase.

Agreed on leaving the hoist out of this PR. Happy to open that separately later if there's interest — your point about download_installer.ts's curl/DNS fallback never having actually run on Windows is a good reason to give that its own PR (and its own test coverage) rather than bundling it in here.

@djkees

djkees commented Sep 30, 2026

Copy link
Copy Markdown
Author

CI's npm audit failure here is unrelated to this PR — a high-severity CVE in a transitive dependency (brace-expansion) published yesterday, affecting develop too. Filed as #277.

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.

2 participants