Allow users to input git credentials without prompts overwriting each…

… other (#775)

* Prevent git password prompts from interfering with each other

If a git repository cannot be accessed in parallel, try it sequentially while
allowing the user to input their credentials. This fixes the issue where
multiple git processes running in parallel would all present a username and
password prompt, overwriting each other.

Avoiding password prompts entirely should be out of scope for aliBuild, as that
requires changing the user's git configuration. Instead, the documentation
should specify how to cache passwords or use SSH auth instead.

* Document ways to cache git credentials

While aliBuild doesn't fail as badly when presenting git credential prompts any
more, it can still be annoying to be prompted often for the same password.

To alleviate this, document ways to cache or eliminate passwords that must be
typed by the user. aliBuild should not do this automatically as it interferes
with the user's configuration.

* Link to troubleshooting docs in aliBuild message

This should make ways to fix the issue of users being prompted for their
password too often easier to find.

* Add unittest covering git wrapper function

* Fix git unittest

In GitHub CI, we have access to the alisw org, so no password prompt is shown.

Instead, just check for the error type, and use another private repo to check
for access restrictions.

alibuild_helpers/build.py

@@ -13,7 +13,7 @@
 from alibuild_helpers.utilities import Hasher
 from alibuild_helpers.utilities import yamlDump
 from alibuild_helpers.utilities import resolve_tag, resolve_version
-from alibuild_helpers.git import git, partialCloneFilter
+from alibuild_helpers.git import git, clone_speedup_options
 from alibuild_helpers.sync import (NoRemoteSync, HttpRemoteSync, S3RemoteSync,
                                    Boto3RemoteSync, RsyncRemoteSync)
 import yaml
@@ -30,6 +30,7 @@
 except ImportError:
   from pipes import quote  # Python 2.7
 
+import concurrent.futures
 import importlib
 import socket
 import os
@@ -52,6 +53,69 @@ def readHashFile(fn):
     return "0"
 
 
+def update_git_repos(args, specs, buildOrder, develPkgs):
+    """Update and/or fetch required git repositories in parallel.
+
+    If any repository fails to be fetched, then it is retried, while allowing the
+    user to input their credentials if required.
+    """
+
+    def update_repo(package, git_prompt):
+        updateReferenceRepoSpec(args.referenceSources, package, specs[package],
+                                fetch=args.fetchRepos,
+                                usePartialClone=not args.docker,
+                                allowGitPrompt=git_prompt)
+
+        # Retrieve git heads
+        cmd = ["ls-remote", "--heads", "--tags"]
+        if package in develPkgs:
+            specs[package]["source"] = \
+                os.path.join(os.getcwd(), specs[package]["package"])
+            cmd.append(specs[package]["source"])
+        else:
+            cmd.append(specs[package].get("reference", specs[package]["source"]))
+
+        output = git(cmd, prompt=git_prompt)
+        specs[package]["git_refs"] = {
+            git_ref: git_hash for git_hash, sep, git_ref
+            in (line.partition("\t") for line in output.splitlines()) if sep
+        }
+
+    requires_auth = set()
+    with concurrent.futures.ThreadPoolExecutor(max_workers=4) as executor:
+        future_to_download = {
+          executor.submit(update_repo, package, git_prompt=False): package
+          for package in buildOrder if "source" in specs[package]
+        }
+        for future in concurrent.futures.as_completed(future_to_download):
+            futurePackage = future_to_download[future]
+            try:
+                future.result()
+            except RuntimeError as exc:
+                # Git failed. Let's assume this is because the user needs to
+                # supply a password.
+                debug("%r requires auth; will prompt later", futurePackage)
+                requires_auth.add(futurePackage)
+            except Exception as exc:
+                raise RuntimeError("Error on fetching %r: %s. Aborting." %
+                                   (futurePackage, exc))
+            else:
+                debug("%r package updated: %d refs found", futurePackage,
+                      len(specs[futurePackage]["git_refs"]))
+
+    # Now execute git commands for private packages one-by-one, so the user can
+    # type their username and password without multiple prompts interfering.
+    for package in requires_auth:
+        banner("If prompted now, enter your username and password for %s below\n"
+               "If you are prompted too often, see: "
+               "https://alisw.github.io/alibuild/troubleshooting.html"
+               "#alibuild-keeps-asking-for-my-password",
+               specs[package]["source"])
+        update_repo(package, git_prompt=True)
+        debug("%r package updated: %d refs found", package,
+              len(specs[package]["git_refs"]))
+
+
 # Creates a directory in the store which contains symlinks to the package
 # and its direct / indirect dependencies
 def createDistLinks(spec, specs, args, syncHelper, repoType, requiresType):
@@ -377,33 +441,7 @@ def doBuild(args, parser):
            os.getcwd(), star())
 
   # Clone/update repos
-  import concurrent.futures
-  with concurrent.futures.ThreadPoolExecutor(max_workers=2) as executor:
-    def downloadTask(p):
-      updateReferenceRepoSpec(args.referenceSources, p, specs[p], args.fetchRepos, not args.docker)
-
-      # Retrieve git heads
-      cmd = ("ls-remote", "--heads", "--tags",
-             specs[p].get("reference", specs[p]["source"]))
-      if specs[p]["package"] in develPkgs:
-         specs[p]["source"] = join(os.getcwd(), specs[p]["package"])
-         cmd = "ls-remote", "--heads", "--tags", specs[p]["source"]
-      output = git(cmd)
-      specs[p]["git_refs"] = {git_ref: git_hash for git_hash, sep, git_ref in
-                              (line.partition("\t") for line in output.splitlines())
-                              if sep}
-      return "%d refs found" % len(specs[p]["git_refs"])
-    future_to_download = {executor.submit(downloadTask, p): p
-                          for p in buildOrder if "source" in specs[p]}
-    for future in concurrent.futures.as_completed(future_to_download):
-        futurePackage = future_to_download[future]
-        try:
-            data = future.result()
-        except Exception as exc:
-            raise RuntimeError("Error on fetching %r: %s. Aborting." %
-                               (futurePackage, exc))
-        else:
-            debug("%r package updated: %s", futurePackage, data)
+  update_git_repos(args, specs, buildOrder, develPkgs)
 
   # Resolve the tag to the actual commit ref
   for p in buildOrder:
@@ -900,10 +938,6 @@ def downloadTask(p):
     if "reference" in spec:
       referenceStatement = "export GIT_REFERENCE=${GIT_REFERENCE_OVERRIDE:-%s}/%s" % (dirname(spec["reference"]), basename(spec["reference"]))
 
-    partialCloneStatement = ""
-    if partialCloneFilter and not args.docker:
-      partialCloneStatement = "export GIT_PARTIAL_CLONE_FILTER='--filter=blob:none'"
-
     debug("spec = %r", spec)
 
     cmd_raw = ""
@@ -944,20 +978,18 @@ def downloadTask(p):
                  sourceDir=source and (dirname(source) + "/") or "",
                  sourceName=source and basename(source) or "",
                  referenceStatement=referenceStatement,
-                 partialCloneStatement=partialCloneStatement,
+                 gitOptionsStatement="" if args.docker else
+                   "export GIT_CLONE_SPEEDUP=" + quote(" ".join(clone_speedup_options())),
                  requires=" ".join(spec["requires"]),
                  build_requires=" ".join(spec["build_requires"]),
                  runtime_requires=" ".join(spec["runtime_requires"])
                 )
 
-    commonPath = "%s/%%s/%s/%s/%s-%s" % (workDir,
-                                         args.architecture,
-                                         spec["package"],
-                                         spec["version"],
-                                         spec["revision"])
-    scriptDir = commonPath % "SPECS"
+    scriptDir = join(workDir, "SPECS", args.architecture, spec["package"],
+                     spec["version"] + "-" + spec["revision"])
 
     err, out = getstatusoutput("mkdir -p %s" % scriptDir)
+    dieOnError(err, "Failed to create script dir %s: %s" % (scriptDir, out))
     writeAll("%s/build.sh" % scriptDir, cmd)
     writeAll("%s/%s.sh" % (scriptDir, spec["package"]), spec["recipe"])
 

alibuild_helpers/build_template.sh

@@ -86,7 +86,7 @@ fi
 
 # Reference statements
 %(referenceStatement)s
-%(partialCloneStatement)s
+%(gitOptionsStatement)s
 
 if [ -z "$CACHED_TARBALL" ]; then
   case "$SOURCE0" in
@@ -105,7 +105,7 @@ if [ -z "$CACHED_TARBALL" ]; then
       else
         # In case there is a stale link / file, for whatever reason.
         rm -rf "$SOURCEDIR"
-        git clone -n $GIT_PARTIAL_CLONE_FILTER ${GIT_REFERENCE:+--reference "$GIT_REFERENCE"} "$SOURCE0" "$SOURCEDIR"
+        git clone -n $GIT_CLONE_SPEEDUP ${GIT_REFERENCE:+--reference "$GIT_REFERENCE"} "$SOURCE0" "$SOURCEDIR"
         cd "$SOURCEDIR"
         git remote set-url --push origin "$WRITE_REPO"
         git checkout -f "$GIT_TAG"

alibuild_helpers/git.py

@@ -6,15 +6,15 @@
 from alibuild_helpers.log import debug
 
 
-def __partialCloneFilter():
-  err, out = getstatusoutput("LANG=C git clone --filter=blob:none 2>&1 | grep 'unknown option'")
-  return err and "--filter=blob:none" or ""
+def clone_speedup_options():
+  """Return a list of options supported by the system git which speed up cloning."""
+  _, out = getstatusoutput("LANG=C git clone --filter=blob:none")
+  if "unknown option" not in out and "invalid filter-spec" not in out:
+    return ["--filter=blob:none"]
+  return []
 
 
-partialCloneFilter = __partialCloneFilter()
-
-
-def git(args, directory=".", check=True):
+def git(args, directory=".", check=True, prompt=True):
   debug("Executing git %s (in directory %s)", " ".join(args), directory)
   # We can't use git --git-dir=%s/.git or git -C %s here as the former requires
   # that the directory we're inspecting to be the root of a git directory, not
@@ -24,9 +24,11 @@ def git(args, directory=".", check=True):
   err, output = getstatusoutput("""\
   set -e +x
   cd {directory} >/dev/null 2>&1
-  exec git {args}
+  {prompt_var} git {args}
   """.format(directory=quote(directory),
-             args=" ".join(map(quote, args))))
+             args=" ".join(map(quote, args)),
+             # GIT_TERMINAL_PROMPT is only supported in git 2.3+.
+             prompt_var="GIT_TERMINAL_PROMPT=0" if not prompt else ""))
   if check and err != 0:
     raise RuntimeError("Error {} from git {}: {}".format(err, " ".join(args), output))
   return output if check else (err, output)

alibuild_helpers/init.py

@@ -1,4 +1,3 @@
-from alibuild_helpers.cmd import execute
 from alibuild_helpers.git import git
 from alibuild_helpers.utilities import getPackageList, parseDefaults, readDefaults, validateDefaults
 from alibuild_helpers.log import debug, error, warning, banner, info
@@ -8,10 +7,6 @@
 from os.path import join
 import os.path as path
 import os, sys
-try:
-  from collections import OrderedDict
-except ImportError:
-  from ordereddict import OrderedDict
 
 def parsePackagesDefinition(pkgname):
   return [ dict(zip(["name","ver"], y.split("@")[0:2]))

alibuild_helpers/workarea.py

@@ -8,10 +8,11 @@
   from ordereddict import OrderedDict
 
 from alibuild_helpers.log import dieOnError, debug, info
-from alibuild_helpers.git import git, partialCloneFilter
+from alibuild_helpers.git import git, clone_speedup_options
 
 
-def updateReferenceRepoSpec(referenceSources, p, spec, fetch, usePartialClone=True):
+def updateReferenceRepoSpec(referenceSources, p, spec,
+                            fetch=True, usePartialClone=True, allowGitPrompt=True):
   """
   Update source reference area whenever possible, and set the spec's "reference"
   if available for reading.
@@ -21,11 +22,14 @@ def updateReferenceRepoSpec(referenceSources, p, spec, fetch, usePartialClone=Tr
   @spec             : the spec of the package to be updated (an OrderedDict)
   @fetch            : whether to fetch updates: if False, only clone if not found
   """
-  spec["reference"] = updateReferenceRepo(referenceSources, p, spec, fetch, usePartialClone)
+  spec["reference"] = updateReferenceRepo(referenceSources, p, spec, fetch,
+                                          usePartialClone, allowGitPrompt)
   if not spec["reference"]:
     del spec["reference"]
 
-def updateReferenceRepo(referenceSources, p, spec, fetch=True, usePartialClone=True):
+
+def updateReferenceRepo(referenceSources, p, spec,
+                        fetch=True, usePartialClone=True, allowGitPrompt=True):
   """
   Update source reference area, if possible.
   If the area is already there and cannot be written, assume it maintained
@@ -64,11 +68,11 @@ def updateReferenceRepo(referenceSources, p, spec, fetch=True, usePartialClone=T
 
   if not os.path.exists(referenceRepo):
     cmd = ["clone", "--bare", spec["source"], referenceRepo]
-    if usePartialClone and partialCloneFilter:
-      cmd.append(partialCloneFilter)
+    if usePartialClone:
+      cmd.extend(clone_speedup_options())
     # This might take a long time, so show the user what's going on.
     info("Cloning git repository for %s...", spec["package"])
-    git(cmd)
+    git(cmd, prompt=allowGitPrompt)
     info("Done cloning git repository for %s", spec["package"])
   elif fetch:
     with codecs.open(os.path.join(os.path.dirname(referenceRepo),
@@ -78,7 +82,8 @@ def updateReferenceRepo(referenceSources, p, spec, fetch=True, usePartialClone=T
       info("Updating git repository for %s...", spec["package"])
       err, output = git(("fetch", "-f", "--tags", spec["source"],
                          "+refs/heads/*:refs/heads/*"),
-                        directory=referenceRepo, check=False)
+                        directory=referenceRepo, check=False,
+                        prompt=allowGitPrompt)
       logf.write(output)
       debug(output)
       dieOnError(err, "Error while updating reference repo for %s." % spec["source"])

docs/troubleshooting.markdown

@@ -299,3 +299,35 @@ This means that you need to do (only once):
 and then adapt you PATH to pickup the local installation, e.g. via:
 
     export PATH=~/.local/bin:$PATH
+
+
+### aliBuild keeps asking for my password
+
+Some packages you may need to build have their source code in a protected repository on CERN GitLab.
+This means that you may be asked for a username and password when you run `aliBuild build`.
+See below for ways to avoid being prompted too often.
+
+#### SSH authentication
+
+You can use an SSH key to authenticate with CERN GitLab.
+This way, you will not be prompted for your GitLab password at all.
+To do this, find your public key (this usually lives in `~/.ssh/id_rsa.pub`) and copy the contents of the file into [your user settings on CERN GitLab][gitlab-ssh-key].
+If you have no SSH key, you can generate one using the `ssh-keygen` command.
+Then, configure git to use SSH to authenticate with CERN GitLab using the following command:
+
+```bash
+git config --global 'url.ssh://git@gitlab.cern.ch:7999/.insteadof' 'https://gitlab.cern.ch/'
+```
+
+[gitlab-ssh-key]: https://gitlab.cern.ch/-/profile/keys
+
+#### Caching passwords
+
+If you prefer not to use SSH keys as described above, you can alternatively configure git to remember the passwords you input for a short time (such as a few hours).
+In order to do this, run the command below (which remembers your passwords for an hour each time you type them into git).
+
+```bash
+git config --global credential.helper 'cache --timeout 3600'
+```
+
+You can adjust the timeout (3600 seconds, above) to your liking, if you would prefer git to remember your passwords for longer.

tests/test_build.py

@@ -107,7 +107,7 @@
                        "+refs/heads/*:refs/heads/*"), "/sw/MIRROR/root", False
 
 
-def dummy_git(args, directory=".", check=True):
+def dummy_git(args, directory=".", check=True, prompt=True):
     return {
         (("symbolic-ref", "-q", "HEAD"), "/alidist", False): (0, "master"),
         (("rev-parse", "HEAD"), "/alidist", True): "6cec7b7b3769826219dfa85e5daa6de6522229a0",
@@ -187,11 +187,11 @@ def dummy_exists(x):
     }.get(x, DEFAULT)
 
 
-git_mock = MagicMock(partialCloneFilter="--filter=blob:none")
-sys.modules["alibuild_helpers.git"] = git_mock
-
-
 # A few errors we should handle, together with the expected result
+@patch("alibuild_helpers.build.clone_speedup_options",
+       new=MagicMock(return_value=["--filter=blob:none"]))
+@patch("alibuild_helpers.workarea.clone_speedup_options",
+       new=MagicMock(return_value=["--filter=blob:none"]))
 class BuildTestCase(unittest.TestCase):
     @patch("alibuild_helpers.analytics", new=MagicMock())
     @patch("requests.Session.get", new=MagicMock())
@@ -275,7 +275,8 @@ def test_coverDoBuild(self, mock_debug, mock_glob, mock_sys, mock_workarea_git):
         exit_code = doBuild(args, mock_parser)
         self.assertEqual(exit_code, 0)
         mock_debug.assert_called_with("Everything done")
-        mock_workarea_git.assert_called_once_with(list(GIT_CLONE_ZLIB_ARGS[0]))
+        mock_workarea_git.assert_called_once_with(list(GIT_CLONE_ZLIB_ARGS[0]),
+                                                  prompt=False)
 
         # Force fetching repos
         mock_workarea_git.reset_mock()