Merge pull request #1777 from buildtesters/improvements_to_builder_cl…

…ass_and_support_for_strict_mode

Improvements to Builder Class and improvement to container support and enable strict mode for tests

bash_completion.sh

@@ -190,7 +190,7 @@ _buildtest ()
  case "$next" in
  build|bd)
  local shortoption="-b -e -et -f -m -n -s -t -u -x -xt"
- local longoption="--buildspec --dry-run --executor --executor-type --exclude --exclude-tags --filter --helpfilter --limit --maxpendtime --max-jobs --modules --module-purge --name --nodes --pollinterval --procs --profile --rerun --remove-stagedir --retry --save-profile --tags --timeout --unload-modules --validate --write-config-file"
+ local longoption="--buildspec --dry-run --executor --executor-type --exclude --exclude-tags --filter --helpfilter --limit --maxpendtime --max-jobs --modules --module-purge --name --nodes --pollinterval --procs --profile --rerun --remove-stagedir --retry --save-profile --strict --tags --timeout --unload-modules --validate --write-config-file"
  local allopts="${longoption} ${shortoption}"
 
  COMPREPLY=( $( compgen -W "$allopts" -- "${cur}" ) )

buildtest/builders/script.py

@@ -3,10 +3,9 @@
 import shutil
 
 from buildtest.builders.base import BuilderBase
-from buildtest.exceptions import BuildTestError
 from buildtest.tools.modules import get_module_commands
 from buildtest.utils.file import write_file
-from buildtest.utils.tools import check_binaries, deep_get
+from buildtest.utils.tools import check_container_runtime, deep_get
 
 
 class ScriptBuilder(BuilderBase):
@@ -26,6 +25,7 @@ def __init__(
  numprocs=None,
  numnodes=None,
  compiler=None,
+ strict=None,
  ):
  super().__init__(
  name=name,
@@ -41,7 +41,7 @@ def __init__(
  self.compiler_settings = {"vars": None, "env": None, "modules": None}
 
  self.configuration = configuration
-
+ self.strict = strict
  self.compiler_section = self.recipe.get("compilers")
 
  # if 'compilers' property defined resolve compiler logic
@@ -132,7 +132,8 @@ def generate_script(self):
  if data_warp_lines:
  script_lines += data_warp_lines
 
- script_lines.append(self._emit_set_command())
+ if self.strict:
+ script_lines.append(self._emit_set_command())
 
  if self.shell.name == "python":
  script_lines = ["#!/bin/bash"]
@@ -196,13 +197,12 @@ def _get_container_command(self):
  container_runtime = container_config["platform"]
  container_command = []
 
+ container_path = check_container_runtime(container_runtime, self.configuration)
  container_launch_command = {
- "docker": ["docker", "run", "-v"],
- "podman": ["podman", "run", "-v"],
- "singularity": ["singularity", "run", "-B"],
+ "docker": [container_path, "run", "--rm", "-v"],
+ "podman": [container_path, "run", "--rm", "-v"],
+ "singularity": [container_path, "run", "-B"],
  }
-
- self._check_binary_path(container_runtime)
  container_command.extend(container_launch_command[container_runtime])
  container_command.append(f"{self.stage_dir}:/buildtest")
 
@@ -219,16 +219,3 @@ def _get_container_command(self):
  container_command.append(container_config["command"])
 
  return container_command
-
- def _check_binary_path(self, platform):
- binary_path = check_binaries(
- [platform],
- custom_dirs=deep_get(self.configuration.target_config, "paths", platform),
- )
-
- if not binary_path[platform]:
- raise BuildTestError(
- f"[blue]{self}[/blue]: [red]Unable to find {platform} binary in PATH, this test will be not be executed.[/red]"
- )
-
- return binary_path[platform]

buildtest/builders/spack.py

@@ -27,6 +27,7 @@ def __init__(
  testdir=None,
  numprocs=None,
  numnodes=None,
+ strict=None,
  ):
  super().__init__(
  name=name,
@@ -38,6 +39,7 @@ def __init__(
  numprocs=numprocs,
  numnodes=numnodes,
  )
+ self.strict = strict
  self.status = deep_get(
  self.recipe, "executors", self.executor, "status"
  ) or self.recipe.get("status")
@@ -54,7 +56,8 @@ def generate_script(self):
  if sched_lines:
  lines += sched_lines
 
- lines.append(self._emit_set_command())
+ if self.strict:
+ lines.append(self._emit_set_command())
 
  var_lines = self._get_variables(self.recipe.get("vars"))
  env_lines = self._get_environment(self.recipe.get("env"))

buildtest/buildsystem/builders.py

@@ -34,6 +34,7 @@ def __init__(
  numnodes=None,
  executor_type=None,
  exclude_tags=None,
+ strict=None,
  ):
  """Based on a loaded Buildspec file, return the correct builder
  for each based on the type. Each type is associated with a known
@@ -50,6 +51,7 @@ def __init__(
  numnodes (list, optional): List of processor values to create builder objects specified via ``buildtest build --nodes``
  executor_type (str, optional): Filter test by executor type (local, batch)
  exclude_tags (list, optional): List of tags to exclude tests from buildspec file
+ strict (bool, optional): This is a boolean used for enable strict mode for test that will run the 'set' command in test.
  """
 
  self.configuration = configuration
@@ -62,7 +64,7 @@ def __init__(
  self.numnodes = numnodes
  self.executor_type = executor_type
  self.exclude_tags = exclude_tags
-
+ self.strict = strict
  self.bp = bp
  self.bc = buildtest_compilers
  self.filters = filters
@@ -169,6 +171,7 @@ def create_script_builders(
  configuration=self.configuration,
  testdir=self.testdir,
  compiler=compiler_name,
+ strict=self.strict,
  )
  builders.append(builder)
 
@@ -184,6 +187,7 @@ def create_script_builders(
  testdir=self.testdir,
  numnodes=node,
  compiler=compiler_name,
+ strict=self.strict,
  )
  builders.append(builder)
  if procs:
@@ -198,6 +202,7 @@ def create_script_builders(
  testdir=self.testdir,
  numprocs=proc,
  compiler=compiler_name,
+ strict=self.strict,
  )
  builders.append(builder)
 
@@ -222,6 +227,7 @@ def create_spack_builders(self, name, recipe, executor, nodes=None, procs=None):
  buildspec=self.bp.buildspec,
  buildexecutor=self.buildexecutor,
  testdir=self.testdir,
+ strict=self.strict,
  )
  builders.append(builder)
 
@@ -235,6 +241,7 @@ def create_spack_builders(self, name, recipe, executor, nodes=None, procs=None):
  buildexecutor=self.buildexecutor,
  testdir=self.testdir,
  numnodes=node,
+ strict=self.strict,
  )
  builders.append(builder)
  if procs:
@@ -247,6 +254,7 @@ def create_spack_builders(self, name, recipe, executor, nodes=None, procs=None):
  buildexecutor=self.buildexecutor,
  testdir=self.testdir,
  numprocs=proc,
+ strict=self.strict,
  )
  builders.append(builder)
 

buildtest/cli/__init__.py

@@ -952,6 +952,13 @@ def build_menu(self):
  "help": "Save buildtest command options into a profile and update configuration file"
  },
  ),
+ (
+ ["--strict"],
+ {
+ "action": "store_true",
+ "help": "Enable strict mode for test by setting 'set -eo pipefail' in test script",
+ },
+ ),
  (
  ["--testdir"],
  {

buildtest/cli/build.py

@@ -626,6 +626,7 @@ def __init__(
  save_profile=None,
  profile=None,
  max_jobs=None,
+ strict=None,
  verbose=None,
  write_config_file=None,
  ):
@@ -665,6 +666,7 @@ def __init__(
  save_profile (str, optional): Save profile to buildtest configuration specified by ``buildtest build --save-profile``
  profile (str, optional): Profile to load from buildtest configuration specified by ``buildtest build --profile``
  max_jobs (int, optional): Maximum number of jobs to run concurrently. This option is specified by ``buildtest build --max-jobs``
+ strict (bool, optional): Enable strict mode for buildtest. This option is specified by ``buildtest build --strict``
  verbose (bool, optional): Enable verbose output for buildtest that is specified by ``buildtest --verbose``
  write_config_file (str, optional): Write configuration file to specified location. This is specified by ``buildtest build --write-config-file``
  """
@@ -742,6 +744,7 @@ def __init__(
  self.save_profile = save_profile
  self.profile = profile
  self.max_jobs = max_jobs
+ self.strict = strict
  self.write_config_file = write_config_file
 
  # this variable contains the detected buildspecs that will be processed by buildtest.
@@ -893,6 +896,7 @@ def load_rerun_file(self):
  self.timeout = content["timeout"]
  self.limit = content["limit"]
  self.max_jobs = content["max_jobs"]
+ self.strict = content["strict"]
 
  def save_rerun_file(self):
  """Record buildtest command options and save them into rerun file which is read when invoking ``buildtest build --rerun``."""
@@ -924,6 +928,7 @@ def save_rerun_file(self):
  "timeout": self.timeout,
  "limit": self.limit,
  "max_jobs": self.max_jobs,
+ "strict": self.strict,
  }
 
  with open(BUILDTEST_RERUN_FILE, "w") as fd:
@@ -949,6 +954,10 @@ def save_profile_to_configuration(self):
  raise BuildTestError(
  f"[red]Configuration file {config_file_path} already exists. Please specify a new file path"
  )
+ if not os.path.splitext(config_file_path)[1] == ".yml":
+ raise BuildTestError(
+ f"[red]Configuration file {config_file_path} must end in .yml extension"
+ )
 
  config_file_path = config_file_path or self.configuration.file
  resolved_buildspecs = []
@@ -980,6 +989,7 @@ def save_profile_to_configuration(self):
  "executor-type": self.executor_type,
  "max_jobs": self.max_jobs,
  "remove-stagedir": self.remove_stagedir,
+ "strict": self.strict,
  }
  # we need to set module-purge to None if it is False. We delete all keys that are 'None' before writing to configuration file
  profile_configuration["module-purge"] = (
@@ -1063,6 +1073,7 @@ def load_profile(self):
  self.executor_type = profile_configuration.get("executor-type")
  self.max_jobs = profile_configuration.get("max_jobs")
  self.remove_stagedir = profile_configuration.get("remove-stagedir")
+ self.strict = profile_configuration.get("strict")
 
  def _validate_filters(self):
  """Check filter fields provided by ``buildtest build --filter`` are valid types and supported. Currently
@@ -1207,6 +1218,7 @@ def parse_buildspecs(self):
  numnodes=self.numnodes,
  executor_type=self.executor_type,
  exclude_tags=self.exclude_tags,
+ strict=self.strict,
  )
 
  if not builder.get_builders():

buildtest/main.py

@@ -325,6 +325,7 @@ def handle_build_command(args, configuration, report_file):
  save_profile=args.save_profile,
  profile=args.profile,
  max_jobs=args.max_jobs,
+ strict=args.strict,
  verbose=args.verbose,
  write_config_file=args.write_config_file,
  )

buildtest/schemas/definitions.schema.json

@@ -672,10 +672,6 @@
  "$ref": "#/definitions/list_of_strings",
  "description": "This field is used for specifying #BSUB options in test script."
  },
- "cobalt": {
- "$ref": "#/definitions/list_of_strings",
- "description": "This field is used for specifying #COBALT options in test script."
- },
  "pbs": {
  "$ref": "#/definitions/list_of_strings",
  "description": "This field is used for specifying #PBS directives in test script."
@@ -703,9 +699,6 @@
  "pbs": {
  "$ref": "#/definitions/list_of_strings"
  },
- "cobalt": {
- "$ref": "#/definitions/list_of_strings"
- },
  "BB": {
  "$ref": "#/definitions/BB"
  },

buildtest/schemas/settings.schema.json

@@ -698,7 +698,8 @@
  "enum": ["local", "batch"]
  },
  "remove-stagedir": {"type": "boolean", "description": "Remove stage directory after test completes"},
- "max-jobs": {"type": "integer", "minimum": 1, "description": "Maximum number of jobs that can be run at a given time for a particular executor"}
+ "max-jobs": {"type": "integer", "minimum": 1, "description": "Maximum number of jobs that can be run at a given time for a particular executor"},
+ "strict": {"type": "boolean", "description": "Enable strict mode for buildtest"}
  }
  },
  "description": {

buildtest/utils/file.py

@@ -77,10 +77,7 @@ def is_symlink(filename: str) -> bool:
  # apply user expansion when file includes something like ~/example
  expanded_filepath = os.path.expanduser(expanded_filepath)
 
- # resolve path will return canonical path eliminating any symbolic links encountered
- resolved_sym_link = resolve_path(filename)
-
- return os.path.islink(expanded_filepath) and resolved_sym_link
+ return os.path.islink(expanded_filepath) and os.path.exists(expanded_filepath)
 
 
 def search_files(

buildtest/utils/tools.py

@@ -5,6 +5,7 @@
 
 from rich.color import Color, ColorParseError
 
+from buildtest.exceptions import BuildTestError
 from buildtest.utils.file import is_dir, resolve_path
 
 logger = logging.getLogger(__name__)
@@ -76,3 +77,24 @@ def check_binaries(binaries, custom_dirs=None):
  logger.debug(f"{command}: {command_fpath}")
 
  return sched_dict
+
+
+def check_container_runtime(platform, configuration):
+ """Check if container runtime exists in $PATH and any additional directories specified by
+ custom_dirs. The return is a dictionary
+
+ Args:
+ platform (str): platform to check for container runtime
+ configuration (dict): configuration dictionary
+ """
+
+ binary_path = check_binaries(
+ [platform], custom_dirs=deep_get(configuration.target_config, "paths", platform)
+ )
+
+ if not binary_path[platform]:
+ raise BuildTestError(
+ f"[red]Unable to find {platform} binary in PATH, this test will be not be executed.[/red]"
+ )
+
+ return binary_path[platform]

docs/gettingstarted/buildingtest.rst

@@ -617,3 +617,32 @@ then proceed to next test.
 .. dropdown:: ``buildtest build -b tutorials/hello_world.yml --rebuild=5 --max-jobs=2``
 
  .. command-output:: buildtest build -b tutorials/hello_world.yml --rebuild=5 --max-jobs=2
+
+Strict Mode
+------------
+
+Buildtest has an option to enable strict mode for test execution which can be enabled via ``--strict`` option. If this
+is set, buildtest will instead ``set -eo pipefail`` in the generated test which will cause test to exit immediately if any
+commands fail. To demonstrate this we have the following buildspec, which runs an **ls** command for an invalid path followed by
+an **echo** command.
+
+.. literalinclude:: ../tutorials/strict_example.yml
+ :language: yaml
+ :emphasize-lines: 8
+
+If we were to run this test without strict mode, we see the test will pass.
+
+.. dropdown:: ``buildtest build -b tutorials/strict_example.yml``
+
+ .. command-output:: buildtest build -b tutorials/strict_example.yml
+
+Now let's run the same test with strict mode enabled, we will see the test will fail with a different return code.
+
+.. dropdown:: ``buildtest build -b tutorials/strict_example.yml --strict``
+
+ .. command-output:: buildtest build -b tutorials/strict_example.yml --strict
+
+ We can see the generated test using **buildtest inspect query -t** and we will see the test script has **set -eo pipefail** in
+ the generated test.
+
+ .. command-output:: buildtest inspect query -t linux_strict_test