From 1427db388524a3bce49fbb8bafb2325759506b1d Mon Sep 17 00:00:00 2001
From: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
Date: Tue, 17 Oct 2023 16:45:39 +0530
Subject: [PATCH] #3049 clarify usage of `cmake` and `casadi`

In the build-time requirements for Windows, we don't use cmake and casadi from pip, but from other sources. This is because we use Visual Studio to compile.
---
 pyproject.toml |  4 ++--
 setup.py       | 23 +++++++++++++----------
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/pyproject.toml b/pyproject.toml
index 9bc84d59e5..df155ac14e 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -2,8 +2,8 @@
 requires = [
     "setuptools",
     "wheel",
+    # On Windows, use the CasADi vcpkg registry and CMake bundled from MSVC
     "casadi>=3.6.0; platform_system!='Windows'",
-    # use CMake bundled from MSVC on Windows
     "cmake; platform_system!='Windows'",
     ]
 build-backend = "setuptools.build_meta"
@@ -72,7 +72,7 @@ examples = [
 # Plotting functionality
 plot = [
     "imageio>=2.9.0",
-    # Note: matplotlib is loaded for debug plots, but to ensure pybamm runs
+    # Note: matplotlib is loaded for debug plots, but to ensure PyBaMM runs
     # on systems without an attached display, it should never be imported
     # outside of plot() methods.
     "matplotlib>=2.0",
diff --git a/setup.py b/setup.py
index 8d1ea327d2..dff6c6b7dc 100644
--- a/setup.py
+++ b/setup.py
@@ -16,13 +16,11 @@
     from distutils.command.build_ext import build_ext
 
 
-# ---------- CMake steps for IDAKLU target (non-Windows) -------------------------------
-
-
 default_lib_dir = (
     "" if system() == "Windows" else os.path.join(os.getenv("HOME"), ".local")
 )
 
+# ---------- set environment variables for vcpkg on Windows ----------------------------
 
 def set_vcpkg_environment_variables():
     if not os.getenv("VCPKG_ROOT_DIR"):
@@ -41,6 +39,7 @@ def set_vcpkg_environment_variables():
         os.getenv("VCPKG_FEATURE_FLAGS"),
     )
 
+# ---------- CMakeBuild class (custom build_ext for IDAKLU target) ---------------------
 
 class CMakeBuild(build_ext):
     user_options = build_ext.user_options + [
@@ -119,6 +118,8 @@ def run(self):
         if os.path.isfile(os.path.join(build_dir, "CMakeError.log")):
             os.remove(os.path.join(build_dir, "CMakeError.log"))
 
+# ---------- configuration for vcpkg on Windows ----------------------------------------
+
         build_env = os.environ
         if os.getenv("PYBAMM_USE_VCPKG"):
             (
@@ -130,26 +131,29 @@ def run(self):
             build_env["vcpkg_default_triplet"] = vcpkg_default_triplet
             build_env["vcpkg_feature_flags"] = vcpkg_feature_flags
 
+# ---------- Run CMake and build IDAKLU module -----------------------------------------
+
         cmake_list_dir = os.path.abspath(os.path.dirname(__file__))
-        print("-" * 10, "Running CMake for idaklu solver", "-" * 40)
+        print("-" * 10, "Running CMake for IDAKLU solver", "-" * 40)
         subprocess.run(
             ["cmake", cmake_list_dir] + cmake_args, cwd=build_dir, env=build_env
-        )
+        , check=True)
 
         if os.path.isfile(os.path.join(build_dir, "CMakeError.log")):
             msg = (
-                "cmake configuration steps encountered errors, and the idaklu module"
+                "cmake configuration steps encountered errors, and the IDAKLU module"
                 " could not be built. Make sure dependencies are correctly "
                 "installed. See "
                 "https://docs.pybamm.org/en/latest/source/user_guide/installation/install-from-source.html" # noqa: E501
             )
             raise RuntimeError(msg)
         else:
-            print("-" * 10, "Building idaklu module", "-" * 40)
+            print("-" * 10, "Building IDAKLU module", "-" * 40)
             subprocess.run(
                 ["cmake", "--build", ".", "--config", "Release"],
                 cwd=build_dir,
                 env=build_env,
+                check=True,
             )
 
             # Move from build temp to final position
@@ -218,7 +222,7 @@ def run(self):
         install.run(self)
 
 
-# ---------- custom wheel build (non-Windows) ------------------------------------------
+# ---------- Custom class for building wheels ------------------------------------------
 
 
 class bdist_wheel(orig.bdist_wheel):
@@ -250,8 +254,7 @@ def compile_KLU():
     # Return True if:
     # - Not running on Windows AND
     # - CMake is found AND
-    # - The pybind11 and casadi-headers directories are found
-    #   in the PyBaMM project directory
+    # - The pybind11/ directory is found in the PyBaMM project directory
     CMakeFound = True
     PyBind11Found = True
     windows = (not system()) or system() == "Windows"