From 478d15f7f4bb48a8442e54d8ab08aaade39e5270 Mon Sep 17 00:00:00 2001
From: Timothee Cour <timothee.cour2@gmail.com>
Date: Thu, 28 Jan 2021 22:51:12 -0800
Subject: [PATCH] improve code in categories.nim; add std/private/gitutils; fix
 flakyness in nim CI (cloneDependency in deps.nim) (#16856)

* improve code in categories.nim; gitutils; fix flakyness in deps.nim
* cleanups
---
 lib/std/private/gitutils.nim | 40 ++++++++++++++++++++++
 testament/categories.nim     | 64 ++++++++++++------------------------
 tools/deps.nim               | 31 +++++++----------
 3 files changed, 73 insertions(+), 62 deletions(-)
 create mode 100644 lib/std/private/gitutils.nim

diff --git a/lib/std/private/gitutils.nim b/lib/std/private/gitutils.nim
new file mode 100644
index 0000000000..bf5e7cb1ff
--- /dev/null
+++ b/lib/std/private/gitutils.nim
@@ -0,0 +1,40 @@
+##[
+internal API for now, API subject to change
+]##
+
+# xxx move other git utilities here; candidate for stdlib.
+
+import std/[os, osproc, strutils]
+
+const commitHead* = "HEAD"
+
+template retryCall*(maxRetry = 3, backoffDuration = 1.0, call: untyped): bool =
+  ## Retry `call` up to `maxRetry` times with exponential backoff and initial
+  ## duraton of `backoffDuration` seconds.
+  ## This is in particular useful for network commands that can fail.
+  runnableExamples:
+    doAssert not retryCall(maxRetry = 2, backoffDuration = 0.1, false)
+    var i = 0
+    doAssert: retryCall(maxRetry = 3, backoffDuration = 0.1, (i.inc; i >= 3))
+    doAssert retryCall(call = true)
+  var result = false
+  var t = backoffDuration
+  for i in 0..<maxRetry:
+    if call:
+      result = true
+      break
+    if i == maxRetry - 1: break
+    sleep(int(t * 1000))
+    t = t * 2 # exponential backoff
+  result
+
+proc isGitRepo*(dir: string): bool =
+  ## This command is used to get the relative path to the root of the repository.
+  ## Using this, we can verify whether a folder is a git repository by checking
+  ## whether the command success and if the output is empty.
+  let (output, status) = execCmdEx("git rev-parse --show-cdup", workingDir = dir)
+  # On Windows there will be a trailing newline on success, remove it.
+  # The value of a successful call typically won't have a whitespace (it's
+  # usually a series of ../), so we know that it's safe to unconditionally
+  # remove trailing whitespaces from the result.
+  result = status == 0 and output.strip() == ""
diff --git a/testament/categories.nim b/testament/categories.nim
index f892076423..472c7f2632 100644
--- a/testament/categories.nim
+++ b/testament/categories.nim
@@ -13,6 +13,7 @@
 # included from testament.nim
 
 import important_packages
+import std/strformat
 
 const
   specialCategories = [
@@ -439,16 +440,7 @@ proc makeSupTest(test, options: string, cat: Category): TTest =
   result.options = options
   result.startTime = epochTime()
 
-const maxRetries = 3
-template retryCommand(call): untyped =
-  var res: typeof(call)
-  var backoff = 1
-  for i in 0..<maxRetries:
-    res = call
-    if res.exitCode == QuitSuccess or i == maxRetries-1: break
-    sleep(backoff * 1000)
-    backoff *= 2
-  res
+import std/private/gitutils
 
 proc testNimblePackages(r: var TResults; cat: Category; packageFilter: string, part: PkgPart) =
   if nimbleExe == "":
@@ -470,41 +462,27 @@ proc testNimblePackages(r: var TResults; cat: Category; packageFilter: string, p
       var test = makeSupTest(name, "", cat)
       let buildPath = packagesDir / name
 
+      template tryCommand(cmd: string, workingDir2 = buildPath, reFailed = reInstallFailed, maxRetries = 1): string =
+        var outp: string
+        let ok = retryCall(maxRetry = maxRetries, backoffDuration = 1.0):
+          var status: int
+          (outp, status) = execCmdEx(cmd, workingDir = workingDir2)
+          status == QuitSuccess
+        if not ok:
+          addResult(r, test, targetC, "", cmd & "\n" & outp, reFailed)
+          continue
+        outp
+
       if not dirExists(buildPath):
-        let (cloneCmd, cloneOutput, cloneStatus) = retryCommand execCmdEx2("git", ["clone", url, buildPath])
-        if cloneStatus != QuitSuccess:
-          r.addResult(test, targetC, "", cloneCmd & "\n" & cloneOutput, reInstallFailed)
-          continue
-
+        discard tryCommand("git clone $# $#" % [url.quoteShell, buildPath.quoteShell], workingDir2 = ".", maxRetries = 3)
         if not useHead:
-          let (fetchCmd, fetchOutput, fetchStatus) = retryCommand execCmdEx2("git", ["fetch", "--tags"], workingDir = buildPath)
-          if fetchStatus != QuitSuccess:
-            r.addResult(test, targetC, "", fetchCmd & "\n" & fetchOutput, reInstallFailed)
-            continue
-
-          let (describeCmd, describeOutput, describeStatus) = retryCommand execCmdEx2("git", ["describe", "--tags", "--abbrev=0"], workingDir = buildPath)
-          if describeStatus != QuitSuccess:
-            r.addResult(test, targetC, "", describeCmd & "\n" & describeOutput, reInstallFailed)
-            continue
-
-          let (checkoutCmd, checkoutOutput, checkoutStatus) = retryCommand execCmdEx2("git", ["checkout", describeOutput.strip], workingDir = buildPath)
-          if checkoutStatus != QuitSuccess:
-            r.addResult(test, targetC, "", checkoutCmd & "\n" & checkoutOutput, reInstallFailed)
-            continue
-
-        let (installDepsCmd, installDepsOutput, installDepsStatus) = retryCommand execCmdEx2("nimble", ["install", "--depsOnly", "-y"], workingDir = buildPath)
-        if installDepsStatus != QuitSuccess:
-          r.addResult(test, targetC, "", "installing dependencies failed:\n$ " & installDepsCmd & "\n" & installDepsOutput, reInstallFailed)
-          continue
-
-      let cmdArgs = parseCmdLine(cmd)
-
-      let (buildCmd, buildOutput, status) = execCmdEx2(cmdArgs[0], cmdArgs[1..^1], workingDir = buildPath)
-      if status != QuitSuccess:
-        r.addResult(test, targetC, "", "package test failed\n$ " & buildCmd & "\n" & buildOutput, reBuildFailed)
-      else:
-        inc r.passed
-        r.addResult(test, targetC, "", "", reSuccess)
+          discard tryCommand("git fetch --tags", maxRetries = 3)
+          let describeOutput = tryCommand("git describe --tags --abbrev=0")
+          discard tryCommand("git checkout $#" % [describeOutput.strip.quoteShell])
+        discard tryCommand("nimble install --depsOnly -y", maxRetries = 3)
+      discard tryCommand(cmd, reFailed = reBuildFailed)
+      inc r.passed
+      r.addResult(test, targetC, "", "", reSuccess)
 
     errors = r.total - r.passed
     if errors == 0:
diff --git a/tools/deps.nim b/tools/deps.nim
index 4ff171482e..8ed95b59ad 100644
--- a/tools/deps.nim
+++ b/tools/deps.nim
@@ -1,26 +1,19 @@
-import os, uri, strformat, osproc, strutils
+import os, uri, strformat, strutils
+import std/private/gitutils
 
 proc exec(cmd: string) =
   echo "deps.cmd: " & cmd
   let status = execShellCmd(cmd)
   doAssert status == 0, cmd
 
-proc execEx(cmd: string): tuple[output: string, exitCode: int] =
-  echo "deps.cmd: " & cmd
-  execCmdEx(cmd, {poStdErrToStdOut, poUsePath, poEvalCommand})
-
-proc isGitRepo(dir: string): bool =
-  # This command is used to get the relative path to the root of the repository.
-  # Using this, we can verify whether a folder is a git repository by checking
-  # whether the command success and if the output is empty.
-  let (output, status) = execEx fmt"git -C {quoteShell(dir)} rev-parse --show-cdup"
-  # On Windows there will be a trailing newline on success, remove it.
-  # The value of a successful call typically won't have a whitespace (it's
-  # usually a series of ../), so we know that it's safe to unconditionally
-  # remove trailing whitespaces from the result.
-  result = status == 0 and output.strip() == ""
-
-const commitHead* = "HEAD"
+proc execRetry(cmd: string) =
+  let ok = retryCall(call = block:
+    let status = execShellCmd(cmd)
+    let result = status == 0
+    if not result:
+      echo fmt"failed command: '{cmd}', status: {status}"
+    result)
+  doAssert ok, cmd
 
 proc cloneDependency*(destDirBase: string, url: string, commit = commitHead,
                       appendRepoName = true, allowBundled = false) =
@@ -33,9 +26,9 @@ proc cloneDependency*(destDirBase: string, url: string, commit = commitHead,
   if not dirExists(destDir):
     # note: old code used `destDir / .git` but that wouldn't prevent git clone
     # from failing
-    exec fmt"git clone -q {url} {destDir2}"
+    execRetry fmt"git clone -q {url} {destDir2}"
   if isGitRepo(destDir):
-    exec fmt"git -C {destDir2} fetch -q"
+    execRetry fmt"git -C {destDir2} fetch -q"
     exec fmt"git -C {destDir2} checkout -q {commit}"
   elif allowBundled:
     discard "this dependency was bundled with Nim, don't do anything"