From 1e0031179080fd010eee5488d2b3fce5b2ca0dc7 Mon Sep 17 00:00:00 2001
From: Robin Lambertz <unfiltered@roblab.la>
Date: Sun, 8 Oct 2023 19:37:51 +0200
Subject: [PATCH] Refactor libraries/download.py (#881)

* Avoid warning in git init when downloading libs

* Remove unused --libraries argument

* Take a Path when parsing libraries-dir

* Simplify expression of default libraries_dir

* Ensure the branch name is always a string

* Move git download to a dedicated function

* Emit error if get_library fails to create include dir
---
 backend/libraries/download.py    | 74 +++++++++++++++++---------------
 backend/libraries/libraries.yaml |  2 +-
 2 files changed, 40 insertions(+), 36 deletions(-)

diff --git a/backend/libraries/download.py b/backend/libraries/download.py
index 7d88e6e8..5160fed2 100755
--- a/backend/libraries/download.py
+++ b/backend/libraries/download.py
@@ -21,6 +21,31 @@ import yaml
 logger = logging.getLogger(__name__)
 
 
+def download_git(library_name, library_version, library_dir, git_download_info, force):
+    # Download with git. Get url and ref, and download using git clone.
+    url = git_download_info["url"]
+    branch = str(git_download_info["branch"])
+    logger.debug("Using git to download library at %s branch %s", url, branch)
+
+    # Recreate repository if force is set.
+    if force and library_dir.exists():
+        shutil.rmtree(library_dir)
+        library_dir.mkdir()
+
+    # Make sure the git repo is initialized. If it already exists, this is
+    # essentially a noop.
+    subprocess.run(["git", "init", "-b", branch, str(library_dir)], check=True)
+
+    # Fetch the ref we want to download, and git reset --hard to it.
+    subprocess.run(
+        ["git", "-C", str(library_dir), "fetch", url, f"refs/heads/{branch}"],
+        check=True,
+    )
+    subprocess.run(
+        ["git", "-C", str(library_dir), "reset", "--hard", "FETCH_HEAD"], check=True
+    )
+
+
 def get_library(
     library_name,
     library_version,
@@ -30,42 +55,28 @@ def get_library(
 ):
     logger.info("Processing %s %s", library_name, library_version)
 
-    # fast-fail if we cannot create the download_cache
     library_dir = libraries_dir / library_name / library_version
     library_dir.mkdir(parents=True, exist_ok=True)
 
     if "git" in download_info:
-        # Download with git. Get url and ref, and download using git clone.
-        url = download_info["git"]["url"]
-        branch = download_info["git"]["branch"]
-        logger.debug("Using git to download library at %s branch %s", url, branch)
-
-        # Recreate repository if force is set.
-        if force and library_dir.exists():
-            shutil.rmtree(library_dir)
-            library_dir.mkdir()
-
-        # Make sure the git repo is initialized. If it already exists, this is
-        # essentially a noop.
-        subprocess.run(["git", "init", str(library_dir)], check=True)
-
-        # Fetch the ref we want to download, and git reset --hard to it.
-        subprocess.run(
-            ["git", "-C", str(library_dir), "fetch", url, f"refs/heads/{branch}"],
-            check=True,
+        download_git(
+            library_name, library_version, library_dir, download_info["git"], force
         )
-        subprocess.run(
-            ["git", "-C", str(library_dir), "reset", "--hard", "FETCH_HEAD"], check=True
-        )
-
-        # Ensure we have an 'include' directory. If we don't, something went wrong.
-        return True
     else:
         logger.error(
             f"No supported download methods for library {library_name} {library_version}"
         )
         return False
 
+    include_dir = library_dir / "include"
+    if not include_dir.is_dir():
+        logger.error(
+            f"Failed to download {library_name} {library_version}: {include_dir} does not exist or isn't a directory."
+        )
+        return False
+
+    return True
+
 
 def download_libraries(args, libraries_config):
     to_download = []
@@ -108,13 +119,10 @@ def main():
     )
     parser.add_argument(
         "--libraries-dir",
-        type=str,
+        type=Path,
         default=None,
         help="Directory where libraries will be stored",
     )
-    parser.add_argument(
-        "--libraries", type=str, nargs="+", help="Only run for these libraries"
-    )
     parser.add_argument(
         "--threads", type=int, default=4, help="Number of download threads to use"
     )
@@ -124,12 +132,8 @@ def main():
     if args.verbose:
         logger.setLevel("DEBUG")
 
-    if args.libraries_dir:
-        args.libraries_dir = Path(args.libraries_dir)
-    else:
-        args.libraries_dir = (
-            Path(os.path.dirname(os.path.realpath(__file__))).parent / "libraries"
-        )
+    if args.libraries_dir == None:
+        args.libraries_dir = Path(os.path.dirname(os.path.realpath(__file__)))
 
     libraries_yaml = (
         Path(os.path.dirname(os.path.realpath(__file__))) / f"libraries.yaml"
diff --git a/backend/libraries/libraries.yaml b/backend/libraries/libraries.yaml
index 7ea4fcab..f5fc2a3a 100644
--- a/backend/libraries/libraries.yaml
+++ b/backend/libraries/libraries.yaml
@@ -2,7 +2,7 @@ directx:
   '5.0':
     git:
       url: https://github.com/roblabla/directx-headers
-      branch: 5.0
+      branch: '5.0'
   '8.0':
     git:
       url: https://github.com/roblabla/directx-headers