Merge branch 'master' into 95-specify-absolute-path-to-executable

benchcab/benchcab.py

@@ -194,10 +194,7 @@ def checkout(self):
     def build(self):
         """Endpoint for `benchcab build`."""
         for repo in self.repos:
-            if repo.build_script:
-                repo.custom_build(verbose=self.args.verbose)
-            else:
-                repo.build(modules=self.config["modules"], verbose=self.args.verbose)
+            repo.build(modules=self.config["modules"], verbose=self.args.verbose)
             print(f"Successfully compiled CABLE for realisation {repo.name}")
         print("")
 

benchcab/repository.py

@@ -84,33 +84,39 @@ def svn_info_show_item(self, item: str) -> str:
         )
         return proc.stdout.strip()
 
-    # TODO(Sean) the modules argument should be in the constructor and
-    # `custom_build()` should be a part of `build()`. This is part of
-    # issue #94.
     def build(self, modules: list[str], verbose=False) -> None:
-        """Build CABLE using the default script."""
+        """Build CABLE using the default build script or a custom build script."""
 
-        print(
-            f"Compiling CABLE {'with MPI' if internal.MPI else 'serially'} for "
-            f"realisation {self.name}..."
-        )
+        if self.build_script:
+            print(
+                "Compiling CABLE using custom build script for "
+                f"realisation {self.name}..."
+            )
+        else:
+            print(
+                f"Compiling CABLE {'with MPI' if internal.MPI else 'serially'} for "
+                f"realisation {self.name}..."
+            )
 
-        default_script_path = (
-            self.root_dir / internal.SRC_DIR / self.name / "offline" / "build3.sh"
+        build_script_path = (
+            self.root_dir
+            / internal.SRC_DIR
+            / self.name
+            / (self.build_script if self.build_script else "offline/build3.sh")
         )
 
-        if not default_script_path.is_file():
+        if not build_script_path.is_file():
             raise FileNotFoundError(
-                f"The default build script, {default_script_path}, could not be found. "
+                f"The build script, {build_script_path}, could not be found. "
                 "Do you need to specify a different build script with the "
                 "'build_script' option in config.yaml?",
             )
 
-        tmp_script_path = default_script_path.parent / "tmp-build3.sh"
+        tmp_script_path = build_script_path.parent / "tmp-build.sh"
 
         if verbose:
-            print(f"Copying {default_script_path} to {tmp_script_path}")
-        shutil.copy(default_script_path, tmp_script_path)
+            print(f"Copying {build_script_path} to {tmp_script_path}")
+        shutil.copy(build_script_path, tmp_script_path)
 
         if verbose:
             print(f"chmod +x {tmp_script_path}")
@@ -123,36 +129,18 @@ def build(self, modules: list[str], verbose=False) -> None:
             )
         remove_module_lines(tmp_script_path)
 
-        with chdir(default_script_path.parent), self.modules_handler.load(
+        args: list[str] = []
+        if internal.MPI and self.build_script is None:
+            args.append("mpi")
+
+        with chdir(build_script_path.parent), self.modules_handler.load(
             modules, verbose=verbose
         ):
             self.subprocess_handler.run_cmd(
-                f"./{tmp_script_path.name}" + (" mpi" if internal.MPI else ""),
+                shlex.join([f"./{tmp_script_path.name}", *args]),
                 verbose=verbose,
             )
 
-    def custom_build(self, verbose=False) -> None:
-        """Build CABLE with a script provided in configuration file"""
-
-        if self.build_script is None:
-            # TODO(Sean) it is bad that we are allowing this to fail silently
-            # but this will be fixed once we have a single build function.
-            return
-
-        print(
-            "Compiling CABLE using custom build script for "
-            f"realisation {self.name}..."
-        )
-
-        build_script_path = (
-            self.root_dir / internal.SRC_DIR / self.name / self.build_script
-        )
-
-        with chdir(build_script_path.parent):
-            self.subprocess_handler.run_cmd(
-                f"./{build_script_path.name}", verbose=verbose
-            )
-
 
 def remove_module_lines(file_path: Path) -> None:
     """Remove lines from `file_path` that call the environment modules package."""

docs/user_guide/config_options.md

@@ -64,7 +64,7 @@ The different running modes of `benchcab` are solely dependent on the options us
 
 #### `build_script`
 
-: This key is **optional**. The path to a custom script to build the code in that branch, relative to the name of the branch.  The script specified with this option will run as is, ignoring the entries in the `modules` key of `config.yaml` file.
+: This key is **optional**. The path to a custom shell script to build the code in that branch, relative to the name of the branch. **Note:** any lines in the provided shell script that call the [environment modules API][environment-modules] will be ignored. To specify modules to use for the build script, please specify them using the [`modules`](#`modules`) key.
 : Example: `build_script: offline/build.sh` to specify a build script under `<name_of_branch>/offline/`.
 
 #### `revision`
@@ -137,4 +137,5 @@ science_configurations: [
 [meorg]: https://modelevaluation.org/
 [forty-two-me]: https://modelevaluation.org/experiment/display/urTKSXEsojdvEPwdR
 [five-me]: https://modelevaluation.org/experiment/display/xNZx2hSvn4PMKAa9R
-[f90nml-github]: https://github.com/marshallward/f90nml
+[f90nml-github]: https://github.com/marshallward/f90nml
+[environment-modules]: https://modules.sourceforge.net/

tests/test_repository.py

@@ -110,17 +110,35 @@ def test_svn_info_show_item():
 
 def test_build():
     """Tests for `CableRepository.build()`."""
-    build_script_path = MOCK_CWD / internal.SRC_DIR / "trunk" / "offline" / "build3.sh"
-    build_script_path.parent.mkdir(parents=True)
-    build_script_path.touch()
+    repo_dir = MOCK_CWD / internal.SRC_DIR / "trunk"
+    build_script_path = repo_dir / "offline" / "build3.sh"
+    custom_build_script_path = repo_dir / "my-custom-build.sh"
     mock_modules = ["foo", "bar"]
 
     # Success case: execute the default build command
+    build_script_path.parent.mkdir(parents=True, exist_ok=True)
+    build_script_path.touch(exist_ok=True)
     mock_subprocess = MockSubprocessWrapper()
     mock_environment_modules = MockEnvironmentModules()
     repo = get_mock_repo(mock_subprocess, mock_environment_modules)
     repo.build(mock_modules)
-    assert "./tmp-build3.sh" in mock_subprocess.commands
+    assert "./tmp-build.sh" in mock_subprocess.commands
+    assert (
+        "module load " + " ".join(mock_modules)
+    ) in mock_environment_modules.commands
+    assert (
+        "module unload " + " ".join(mock_modules)
+    ) in mock_environment_modules.commands
+
+    # Success case: execute the build command for a custom build script
+    custom_build_script_path.parent.mkdir(parents=True, exist_ok=True)
+    custom_build_script_path.touch(exist_ok=True)
+    mock_subprocess = MockSubprocessWrapper()
+    mock_environment_modules = MockEnvironmentModules()
+    repo = get_mock_repo(mock_subprocess, mock_environment_modules)
+    repo.build_script = str(custom_build_script_path.relative_to(repo_dir))
+    repo.build(mock_modules)
+    assert "./tmp-build.sh" in mock_subprocess.commands
     assert (
         "module load " + " ".join(mock_modules)
     ) in mock_environment_modules.commands
@@ -129,69 +147,69 @@ def test_build():
     ) in mock_environment_modules.commands
 
     # Success case: test non-verbose standard output
+    build_script_path.parent.mkdir(parents=True, exist_ok=True)
+    build_script_path.touch(exist_ok=True)
+    repo = get_mock_repo()
+    with contextlib.redirect_stdout(io.StringIO()) as buf:
+        repo.build(mock_modules)
+    assert buf.getvalue() == "Compiling CABLE serially for realisation trunk...\n"
+
+    # Success case: test non-verbose standard output for a custom build script
+    custom_build_script_path.parent.mkdir(parents=True, exist_ok=True)
+    custom_build_script_path.touch(exist_ok=True)
     repo = get_mock_repo()
+    repo.build_script = str(custom_build_script_path.relative_to(repo_dir))
     with contextlib.redirect_stdout(io.StringIO()) as buf:
         repo.build(mock_modules)
-    assert buf.getvalue() == ("Compiling CABLE serially for realisation trunk...\n")
+    assert buf.getvalue() == (
+        "Compiling CABLE using custom build script for realisation trunk...\n"
+    )
 
     # Success case: test verbose standard output
+    build_script_path.parent.mkdir(parents=True, exist_ok=True)
+    build_script_path.touch(exist_ok=True)
     repo = get_mock_repo()
     with contextlib.redirect_stdout(io.StringIO()) as buf:
         repo.build(mock_modules, verbose=True)
     assert buf.getvalue() == (
         "Compiling CABLE serially for realisation trunk...\n"
-        f"Copying {build_script_path} to {build_script_path.parent}/tmp-build3.sh\n"
-        f"chmod +x {build_script_path.parent}/tmp-build3.sh\n"
-        "Modifying tmp-build3.sh: remove lines that call environment "
+        f"Copying {build_script_path} to {build_script_path.parent}/tmp-build.sh\n"
+        f"chmod +x {build_script_path.parent}/tmp-build.sh\n"
+        "Modifying tmp-build.sh: remove lines that call environment "
         "modules\n"
         f"Loading modules: {' '.join(mock_modules)}\n"
         f"Unloading modules: {' '.join(mock_modules)}\n"
     )
 
-    # Failure case: cannot find default build script
-    build_script_path.unlink()
+    # Success case: test verbose standard output for a custom build script
+    custom_build_script_path.parent.mkdir(parents=True, exist_ok=True)
+    custom_build_script_path.touch(exist_ok=True)
     repo = get_mock_repo()
-    with pytest.raises(
-        FileNotFoundError,
-        match=f"The default build script, {MOCK_CWD}/src/trunk/offline/build3.sh, "
-        "could not be found. Do you need to specify a different build script with the "
-        "'build_script' option in config.yaml?",
-    ):
-        repo.build(mock_modules)
-
-
-def test_custom_build():
-    """Tests for `custom_build()`."""
-
-    build_script = "offline/build.sh"
-    build_script_path = MOCK_CWD / internal.SRC_DIR / "trunk" / build_script
-    build_script_path.parent.mkdir(parents=True)
-    build_script_path.touch()
-
-    # Success case: execute custom build command
-    mock_subprocess = MockSubprocessWrapper()
-    repo = get_mock_repo(subprocess_handler=mock_subprocess)
-    repo.build_script = build_script
-    repo.custom_build()
-    assert f"./{build_script_path.name}" in mock_subprocess.commands
-
-    # Success case: test non-verbose standard output
-    repo = get_mock_repo()
-    repo.build_script = build_script
+    repo.build_script = str(custom_build_script_path.relative_to(repo_dir))
     with contextlib.redirect_stdout(io.StringIO()) as buf:
-        repo.custom_build()
+        repo.build(mock_modules, verbose=True)
     assert buf.getvalue() == (
         "Compiling CABLE using custom build script for realisation trunk...\n"
+        f"Copying {custom_build_script_path} to {custom_build_script_path.parent}/tmp-build.sh\n"
+        f"chmod +x {custom_build_script_path.parent}/tmp-build.sh\n"
+        "Modifying tmp-build.sh: remove lines that call environment "
+        "modules\n"
+        f"Loading modules: {' '.join(mock_modules)}\n"
+        f"Unloading modules: {' '.join(mock_modules)}\n"
     )
 
-    # Success case: test verbose standard output
+    # Failure case: cannot find default build script
+    build_script_path.parent.mkdir(parents=True, exist_ok=True)
+    build_script_path.touch(exist_ok=True)
+    build_script_path.unlink()
     repo = get_mock_repo()
-    repo.build_script = build_script
-    with contextlib.redirect_stdout(io.StringIO()) as buf:
-        repo.custom_build(verbose=True)
-    assert buf.getvalue() == (
-        "Compiling CABLE using custom build script for realisation trunk...\n"
-    )
+    with pytest.raises(
+        FileNotFoundError,
+        match=f"The build script, {MOCK_CWD}/src/trunk/offline/build3.sh, could not be "
+        "found. Do you need to specify a different build script with the 'build_script' "
+        "option in config.yaml?",
+    ):
+        repo.build(mock_modules)
 
 
 def test_remove_module_lines():