Merge branch 'force-checking-of-error-code' into 'split-installation-tests'

# Conflicts:
#   tests.py

.gitlab-ci.yml

@@ -15,7 +15,7 @@ stages:
   - ls -l
   - pip install -U pip
   - pip --version
-  - pip install pytest black ruff
+  - pip install pytest black ruff pydocstyle
   - pytest --version
   - cat /etc/issue
 
@@ -79,6 +79,9 @@ style:
     - ruff --version
     - ruff .
     - black --check --diff .
+    - pydocstyle mpsd-software-environment.py
+    - pydocstyle tests.py
+    # we could also use `ruff --select D` for pycodestyle. But the behaviour is not exactly the same.
 
 
 test-bullseye:

mpsd-software-environment.py

 #!/usr/bin/env python3
+
+"""mpsd-software-environment: tool for installation of toolchains."""
+
 import argparse
 import datetime
 import os
@@ -15,6 +18,7 @@ This function builds toolchains for MPSD-HPC at the appropriate directory, \n
 for given system architecture and MPSD software stack version.\n
 The toolchains
 are built using the bash script spack_setup.sh, and the results are logged. """
+
 config_vars = {
     "cmd_log_file": "install.log",
     "build_log_file": (
@@ -28,29 +32,32 @@ config_vars = {
 
 # Helper class to change directory via context manager
 class os_chdir:
-    """
-    The os_chdir class is a context manager that
-    changes the current directory to a specified directory
+    """The os_chdir class is a context manager.
+
+    It changes the current directory to a specified directory
     and returns to the original directory after execution.
     """
 
     def __init__(self, new_dir):
+        """Initialize, save original directory."""
         self.new_dir = new_dir
         self.saved_dir = os.getcwd()
 
     def __enter__(self):
+        """Go to target directory (main action for context)."""
         os.chdir(self.new_dir)
 
     def __exit__(self, exc_type, exc_val, exc_tb):
+        """On exist we return to original directory."""
         os.chdir(self.saved_dir)
 
 
 def run(*args, counter=[0], **kwargs):
     """
-    Call subprocess.run(*args, **kwargs) and print logging data.
+    Call subprocess.run and print logging data.
 
-    Conveniene function to call `subprocess.run` and provide some metadata
-    about the call.
+    Conveniene function to call `subprocess.run(*args, **kwargs)`
+    and provide some metadata about the call.
 
     Parameters
     ----------
@@ -130,22 +137,34 @@ def run(*args, counter=[0], **kwargs):
 
 
 def setup_log_cmd(
-    mpsd_release: str, script_dir: str, msg: str = None, *args, **kwargs
+    mpsd_release: str, script_dir: str, msg: str = None, **kwargs
 ) -> None:
-    """The setup_log_cmd function logs the command used to build the
-    toolchains, along with information about the software environment installer
-    branch, the Spack environments branch, and the commit hashes of each. It
-    also logs steps taken in install process using the optional message
-    argument.
+    """
+    Log the command used to build the toolchains.
 
-    Args:
-        mpsd_release (str): The name of the release to install toolchains for.
-        script_dir (str): The path to the directory where the scripts are located.
-        msg (str, optional): An optional message to log in the command log file.
+    It also logs information about the software environment installer branch,
+    the Spack environments branch, and the commit hashes of each.
+    It also logs steps taken
+    in the install process using the optional message argument.
 
-    Returns:
-        None
+    Parameters
+    ----------
+    - mpsd_release : str
+        The name of the release to install toolchains for.
+    - script_dir : str
+        The path to the directory where the scripts are located.
+    - msg : str, optional
+        An optional message to log in the command log file.
+    - **kwargs : dict
+        A dictionary with values for
+        - spe_branch : str
+            The name of the Spack environments branch.
+        - spe_commit_hash : str
+            The commit hash of the Spack environments branch.
 
+    Returns
+    -------
+    - None
     """
     release_base_dir = script_dir / mpsd_release
 
@@ -205,17 +224,21 @@ def setup_log_cmd(
 
 
 def create_dir_structure(mpsd_release: str, script_dir: Path) -> None:
-    """The create_dir_structure function creates the directory structure for
+    """
+    Create the directory structure and clone spack environments repo.
+
+    The create_dir_structure function creates the directory structure for
     the specified release and clones the Spack environments repository if it
     doesn't exist.
 
-    Args:
+    Parameters
+    ----------
     - mpsd_release: A string representing the MPSD release version.
     - script_dir: A Path object representing the path to the scripts directory.
 
-    Returns:
+    Returns
+    -------
     - None
-
     """
     # Create the directory structure for the release
     release_base_dir = script_dir / mpsd_release
@@ -248,22 +271,31 @@ def create_dir_structure(mpsd_release: str, script_dir: Path) -> None:
 
 def get_release_info(mpsd_release: str, script_dir: Path) -> Tuple[str, str, List[str]]:
     """
+    Get information about the specified release.
+
     Get information about the specified release, such as the branch and commit hash
     of the Spack environments repository and the available toolchains.
 
-    Args:
-    - mpsd_release (str) : the name of the release to get information for.
-    - script_dir (pathlib.Path): the base directory where releases are stored.
-
-    Returns:
-    - spe_branch: str, the name of the branch for the Spack environments repository.
-    - spe_commit_hash: str, the commit hash for the Spack environments repository.
-    - available_toolchains: list, a list of strings representing the available
-      toolchains for the release.
+    Parameters
+    ----------
+    mpsd_release : str
+        The name of the release to get information for.
+    script_dir : pathlib.Path
+        The base directory where releases are stored.
 
-    Raises:
-    - FileNotFoundError: If the release directory does not exist. Run
-      `create_dir_structure()` first.
+    Returns
+    -------
+    spe_branch : str
+        The name of the branch for the Spack environments repository.
+    spe_commit_hash : str
+        The commit hash for the Spack environments repository.
+    available_toolchains : list
+        A list of strings representing the available toolchains for the release.
+
+    Raises
+    ------
+    FileNotFoundError
+        If the release directory does not exist. Run `create_dir_structure()` first.
     """
     # Get the info for release
     release_base_dir = script_dir / mpsd_release
@@ -294,20 +326,26 @@ def get_release_info(mpsd_release: str, script_dir: Path) -> Tuple[str, str, Lis
 
 def prepare_environment(mpsd_release: str, script_dir: Path) -> List[str]:
     """
-    - Creates the directory structure for the given MPSD release and clones the
-      spack-environments repository.
-    - Determines the branch and commit hash of the spack-environments repository and
-      the available toolchains.
-    - Logs the command usage.
-
-    Args:
-    - mpsd_release (str): The name of the MPSD release to prepare the environment for.
-    - script_dir (pathlib.Path): The base directory to create the release folder and
-      clone the spack-environments repository into.
-
-    Returns:
-    - available_toolchains (list): A list of available toolchains for the given MPSD
-      release.
+    Create the directory structure for the given MPSD release.
+
+    It does the following steps:
+    Clones the spack-environments repository.
+    Determines the branch and commit hash of the spack-environments repository
+    and the available toolchains.
+    Logs the command usage.
+
+    Parameters
+    ----------
+    mpsd_release : str
+        The name of the MPSD release to prepare the environment for.
+    script_dir : pathlib.Path
+        The base directory to create the release folder and
+        clone the spack-environments repository into.
+
+    Returns
+    -------
+    available_toolchains : list
+        A list of available toolchains for the given MPSD release.
     """
     create_dir_structure(mpsd_release, script_dir)
     spe_branch, spe_commit_hash, available_toolchains = get_release_info(
@@ -326,30 +364,36 @@ def install_environment(
     force_reinstall: bool = False,
     enable_build_cache: bool = False,
 ) -> None:
-    """Installs the specified MPSD release and toolchains to the specified
-    directory using Spack.
-
-    Args:
-        mpsd_release: A string representing the MPSD release version.
-
-        toolchains: A list of strings representing the toolchains to install
-                    (e.g., "foss2021a-mpi", "global_generic", "ALL").
-
-        script_dir: A Path object representing the path to the directory where
-                    the release and toolchains will be installed.
-
-        force_reinstall: A boolean indicating whether to force a reinstallation
-                    even if the release and toolchains already exist. Defaults to False.
-
-        enable_build_cache: A boolean indicating whether to build the build
-                    cache when installing toolchains. Defaults to False.
+    """
+    Install the specified MPSD release and toolchains.
 
-    Raises:
-        ValueError: If a requested toolchain is not available in the specified release.
+    The function installs the toolchain to the specified directory, using Spack.
 
-    Returns:
-        None
+    Parameters
+    ----------
+    mpsd_release : str
+        A string representing the MPSD release version.
+    toolchains : list of str
+        A list of strings representing the toolchains to install
+        (e.g., "foss2021a-mpi", "global_generic", "ALL").
+    script_dir : pathlib.Path
+        A Path object representing the path to the directory where
+        the release and toolchains will be installed.
+    force_reinstall : bool, optional
+        A boolean indicating whether to force a reinstallation
+        even if the release and toolchains already exist. Defaults to False.
+    enable_build_cache : bool, optional
+        A boolean indicating whether to build the build cache
+        when installing toolchains. Defaults to False.
+
+    Raises
+    ------
+    ValueError
+        If a requested toolchain is not available in the specified release.
 
+    Returns
+    -------
+    None
     """
     print(
         f"Installing release {mpsd_release} with toolchains {toolchains} "
@@ -427,18 +471,21 @@ def install_environment(
 
 
 def remove_environment(release, toolchains, target_dir):
+    """Remove release from installation."""
     msg = f"Removing release {release} with toolchains {toolchains} from {target_dir}"
     print(msg)
     raise NotImplementedError(msg)
 
 
 def start_new_environment(release, from_release, target_dir):
+    """Start new MPSD software environment version."""
     msg = f"Starting new release {release} from {from_release} to {target_dir}"
     print(msg)
     raise NotImplementedError(msg)
 
 
 def main():
+    """Execute main entry point."""
     parser = argparse.ArgumentParser(description=about_tool)
     subparsers = parser.add_subparsers(
         dest="action", title="actions", description="valid actions", required=True

tests.py

+"""Tests for mpsd-software-environment.py."""
+
 import importlib
 import os
 import shutil
@@ -45,6 +47,7 @@ def create_mock_git_repository(target_directory, create_directory=True):
 
 
 def test_os_chdir(tmp_path):
+    """Test the os_chdir context manager."""
     # create a temporary directory for testing
     temp_dir = tmp_path / "test_os_chdir"
     temp_dir.mkdir()
@@ -61,11 +64,13 @@ def test_os_chdir(tmp_path):
 
 
 def test_prepare_environment(tmp_path):
-    # simulate running ./install-software-environment.py --release dev-23a \
-    #   --target-directory /tmp/test_prepare_env
-    # prepare_env is run when cmd is not specified, we can test cmd='prepare'
-    # and cmd=None to check both cases
-
+    """Simulate running preparation of environment.
+    
+    Simulate running ./install-software-environment.py --release dev-23a \
+      --target-directory /tmp/test_prepare_env
+    prepare_env is run when cmd is not specified, we can test cmd='prepare'
+    and cmd=None to check both cases
+    """
     script_dir = tmp_path / "mpsd_opt" / "linux_debian_11"
     spack_environments = "spack-environments"
     mpsd_release_to_test = "dev-23a"
@@ -114,7 +119,10 @@ def test_prepare_environment(tmp_path):
 
 
 def test_setup_log_cmd(tmp_path):
-    # check that logs/install-software-environment.log is updated when the module is run
+    """Check that log is updated.
+
+    Check that logs/install-software-environment.log is updated when the module is run
+    """
     log_file = "install.log"
 
     script_dir = tmp_path / "test_prepare_env"
@@ -140,11 +148,13 @@ def test_setup_log_cmd(tmp_path):
 
 
 def test_install_environment_wrong_toolchain(tmp_path):
-    """Test exception is raised for non-existing toolchain."""
-    # This is a long test, its handy to test this with print statements printed to
-    # stdout, use:
-    #   pytest -s
-    # Expect an Exception when wrong toolchains are provided
+    """Test exception is raised for non-existing toolchain.
+
+    This is a long test, its handy to test this with print statements printed to
+    stdout, use:
+      pytest -s
+    Expect an Exception when wrong toolchains are provided
+    """
     with pytest.raises(Exception):
         mod.install_environment(
             mpsd_release="dev-23a",
@@ -210,6 +220,8 @@ def test_install_environment_zlib(tmp_path):
         .stdout.decode("utf-8")
         .strip()
     )
+    assert len(gcc_ver) > 3, f"Couldn't find gcc {gcc_ver=}"
+
     setup_file = release_base_dir / "spack-environments/spack_setup.sh"
     with open(setup_file, "r") as f:
         lines = f.read().replace(
@@ -266,6 +278,7 @@ def test_install_environment_zlib(tmp_path):
 
 
 def test_interface(tmp_path):
+    """Test other things (not implemented yet)."""
     pass
     # ensure that installing without toolchains only passes the available toolchains
     # check that the script branch and hash are correct when running the script