From 762ba27036e717aeceeb1b43f3090f0e9a049869 Mon Sep 17 00:00:00 2001
From: Tom Scogland <tom.scogland@gmail.com>
Date: Wed, 7 Sep 2022 11:12:57 -0700
Subject: Make GHA tests parallel by using xdist (#32361)

* Add two no-op jobs named "all-prechecks" and "all"

These are a suggestion from @tgamblin, they are stable named markers we
can use from gitlab and possibly for required checks to make CI more
resilient to refactors changing the names of specific checks.

* Enable parallel testing using xdist for unit testing in CI

* Normalize tmp paths to deal with macos

* add -u flag compatibility to spack python

As of now, it is accepted and ignored.  The usage with xdist, where it
is invoked specifically by `python -u spack python` which is then passed
`-u` by xdist is the entire reason for doing this.  It should never be
used without explicitly passing -u to the executing python interpreter.

* use spack python in xdist to support python 2

When running on python2, spack has many import cycles unless started
through main.  To allow that, this uses `spack python` as the
interpreter, leveraging the `-u` support so xdist doesn't error out when
it unconditionally requests unbuffered binary IO.

* Use shutil.move to account for tmpdir being in a separate filesystem sometimes
---
 lib/spack/llnl/util/tty/pty.py          |  4 +--
 lib/spack/spack/cmd/python.py           |  6 ++++
 lib/spack/spack/main.py                 | 50 ++++++++++++++++++++++-----------
 lib/spack/spack/test/ci.py              |  1 +
 lib/spack/spack/test/cmd/checksum.py    |  4 ++-
 lib/spack/spack/test/cmd/ci.py          |  4 +--
 lib/spack/spack/test/cmd/commands.py    | 12 ++++----
 lib/spack/spack/test/cmd/env.py         |  2 +-
 lib/spack/spack/test/cmd/help.py        |  6 ++--
 lib/spack/spack/test/cmd/is_git_repo.py | 33 +++++++++++-----------
 lib/spack/spack/test/cmd/module.py      |  4 +--
 lib/spack/spack/test/cmd/style.py       | 50 +++++++++++++++++----------------
 lib/spack/spack/test/cmd_extensions.py  | 11 +++++---
 lib/spack/spack/util/debug.py           |  6 +++-
 14 files changed, 114 insertions(+), 79 deletions(-)

(limited to 'lib')

diff --git a/lib/spack/llnl/util/tty/pty.py b/lib/spack/llnl/util/tty/pty.py
index 8d7213a533..df743a6055 100644
--- a/lib/spack/llnl/util/tty/pty.py
+++ b/lib/spack/llnl/util/tty/pty.py
@@ -228,8 +228,8 @@ class PseudoShell(object):
         self.minion_function = minion_function
 
         # these can be optionally set to change defaults
-        self.controller_timeout = 1
-        self.sleep_time = 0
+        self.controller_timeout = 3
+        self.sleep_time = 0.1
 
     def start(self, **kwargs):
         """Start the controller and minion processes.
diff --git a/lib/spack/spack/cmd/python.py b/lib/spack/spack/cmd/python.py
index 028184b817..5df8b30dde 100644
--- a/lib/spack/spack/cmd/python.py
+++ b/lib/spack/spack/cmd/python.py
@@ -30,6 +30,12 @@ def setup_parser(subparser):
         help="print the Python version number and exit",
     )
     subparser.add_argument("-c", dest="python_command", help="command to execute")
+    subparser.add_argument(
+        "-u",
+        dest="unbuffered",
+        action="store_true",
+        help="for compatibility with xdist, do not use without adding -u to the interpreter",
+    )
     subparser.add_argument(
         "-i",
         dest="python_interpreter",
diff --git a/lib/spack/spack/main.py b/lib/spack/spack/main.py
index f69b1e0088..78a0d47519 100644
--- a/lib/spack/spack/main.py
+++ b/lib/spack/spack/main.py
@@ -18,6 +18,7 @@ import os.path
 import pstats
 import re
 import signal
+import subprocess as sp
 import sys
 import traceback
 import warnings
@@ -623,15 +624,19 @@ class SpackCommand(object):
     their output.
     """
 
-    def __init__(self, command_name):
+    def __init__(self, command_name, subprocess=False):
         """Create a new SpackCommand that invokes ``command_name`` when called.
 
         Args:
             command_name (str): name of the command to invoke
+            subprocess (bool): whether to fork a subprocess or not. Currently not supported on
+                Windows, where it is always False.
         """
         self.parser = make_argument_parser()
         self.command = self.parser.add_command(command_name)
         self.command_name = command_name
+        # TODO: figure out how to support this on windows
+        self.subprocess = subprocess if sys.platform != "win32" else False
 
     def __call__(self, *argv, **kwargs):
         """Invoke this SpackCommand.
@@ -656,25 +661,36 @@ class SpackCommand(object):
         self.error = None
 
         prepend = kwargs["global_args"] if "global_args" in kwargs else []
-
-        args, unknown = self.parser.parse_known_args(prepend + [self.command_name] + list(argv))
-
         fail_on_error = kwargs.get("fail_on_error", True)
 
-        out = StringIO()
-        try:
-            with log_output(out):
-                self.returncode = _invoke_command(self.command, self.parser, args, unknown)
+        if self.subprocess:
+            p = sp.Popen(
+                [spack.paths.spack_script, self.command_name] + prepend + list(argv),
+                stdout=sp.PIPE,
+                stderr=sp.STDOUT,
+            )
+            out, self.returncode = p.communicate()
+            out = out.decode()
+        else:
+            args, unknown = self.parser.parse_known_args(
+                prepend + [self.command_name] + list(argv)
+            )
+
+            out = StringIO()
+            try:
+                with log_output(out):
+                    self.returncode = _invoke_command(self.command, self.parser, args, unknown)
 
-        except SystemExit as e:
-            self.returncode = e.code
+            except SystemExit as e:
+                self.returncode = e.code
 
-        except BaseException as e:
-            tty.debug(e)
-            self.error = e
-            if fail_on_error:
-                self._log_command_output(out)
-                raise
+            except BaseException as e:
+                tty.debug(e)
+                self.error = e
+                if fail_on_error:
+                    self._log_command_output(out)
+                    raise
+            out = out.getvalue()
 
         if fail_on_error and self.returncode not in (None, 0):
             self._log_command_output(out)
@@ -683,7 +699,7 @@ class SpackCommand(object):
                 % (self.returncode, self.command_name, ", ".join("'%s'" % a for a in argv))
             )
 
-        return out.getvalue()
+        return out
 
     def _log_command_output(self, out):
         if tty.is_verbose():
diff --git a/lib/spack/spack/test/ci.py b/lib/spack/spack/test/ci.py
index f42518f7fb..2d8e65e8a5 100644
--- a/lib/spack/spack/test/ci.py
+++ b/lib/spack/spack/test/ci.py
@@ -484,6 +484,7 @@ def test_get_spec_filter_list(mutable_mock_env_path, config, mutable_mock_repo):
     assert affected_pkg_names == expected_affected_pkg_names
 
 
+@pytest.mark.maybeslow
 @pytest.mark.regression("29947")
 def test_affected_specs_on_first_concretization(mutable_mock_env_path, config):
     e = ev.create("first_concretization")
diff --git a/lib/spack/spack/test/cmd/checksum.py b/lib/spack/spack/test/cmd/checksum.py
index b0fffbea08..a84b0eb8b7 100644
--- a/lib/spack/spack/test/cmd/checksum.py
+++ b/lib/spack/spack/test/cmd/checksum.py
@@ -50,12 +50,14 @@ def test_checksum(arguments, expected, mock_packages, mock_stage):
 
 @pytest.mark.skipif(sys.platform == "win32", reason="Not supported on Windows (yet)")
 def test_checksum_interactive(mock_packages, mock_fetch, mock_stage, monkeypatch):
+    # TODO: mock_fetch doesn't actually work with stage, working around with ignoring
+    # fail_on_error for now
     def _get_number(*args, **kwargs):
         return 1
 
     monkeypatch.setattr(tty, "get_number", _get_number)
 
-    output = spack_checksum("preferred-test")
+    output = spack_checksum("preferred-test", fail_on_error=False)
     assert "version of preferred-test" in output
     assert "version(" in output
 
diff --git a/lib/spack/spack/test/cmd/ci.py b/lib/spack/spack/test/cmd/ci.py
index 0ac352c1df..e7e7569fe9 100644
--- a/lib/spack/spack/test/cmd/ci.py
+++ b/lib/spack/spack/test/cmd/ci.py
@@ -2192,10 +2192,8 @@ spack:
 )
 def test_ci_help(subcmd, capsys):
     """Make sure `spack ci` --help describes the (sub)command help."""
-    with pytest.raises(SystemExit):
-        ci_cmd(subcmd, "--help")
+    out = spack.main.SpackCommand("ci", subprocess=True)(subcmd, "--help")
 
-    out = str(capsys.readouterr())
     usage = "usage: spack ci {0}{1}[".format(subcmd, " " if subcmd else "")
     assert usage in out
 
diff --git a/lib/spack/spack/test/cmd/commands.py b/lib/spack/spack/test/cmd/commands.py
index 23794a6bb0..d972f86d7f 100644
--- a/lib/spack/spack/test/cmd/commands.py
+++ b/lib/spack/spack/test/cmd/commands.py
@@ -16,7 +16,7 @@ import spack.main
 import spack.paths
 from spack.cmd.commands import _positional_to_subroutine
 
-commands = spack.main.SpackCommand("commands")
+commands = spack.main.SpackCommand("commands", subprocess=True)
 
 parser = spack.main.make_argument_parser()
 spack.main.add_all_commands(parser)
@@ -104,17 +104,18 @@ _cmd-spack-install:
 
 
 def test_rst_with_header(tmpdir):
+    local_commands = spack.main.SpackCommand("commands")
     fake_header = "this is a header!\n\n"
 
     filename = tmpdir.join("header.txt")
     with filename.open("w") as f:
         f.write(fake_header)
 
-    out = commands("--format=rst", "--header", str(filename))
+    out = local_commands("--format=rst", "--header", str(filename))
     assert out.startswith(fake_header)
 
     with pytest.raises(spack.main.SpackCommandError):
-        commands("--format=rst", "--header", "asdfjhkf")
+        local_commands("--format=rst", "--header", "asdfjhkf")
 
 
 def test_rst_update(tmpdir):
@@ -207,13 +208,14 @@ def test_update_completion_arg(tmpdir, monkeypatch):
 
     monkeypatch.setattr(spack.cmd.commands, "update_completion_args", mock_args)
 
+    local_commands = spack.main.SpackCommand("commands")
     # ensure things fail if --update-completion isn't specified alone
     with pytest.raises(spack.main.SpackCommandError):
-        commands("--update-completion", "-a")
+        local_commands("--update-completion", "-a")
 
     # ensure arg is restored
     assert "--update-completion" not in mock_bashfile.read()
-    commands("--update-completion")
+    local_commands("--update-completion")
     assert "--update-completion" in mock_bashfile.read()
 
 
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index ceefd3323b..f564ca3c80 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -869,7 +869,7 @@ def test_env_with_included_config_var_path(packages_file):
 
     config_real_path = substitute_path_variables(config_var_path)
     fs.mkdirp(os.path.dirname(config_real_path))
-    fs.rename(packages_file.strpath, config_real_path)
+    shutil.move(packages_file.strpath, config_real_path)
     assert os.path.exists(config_real_path)
 
     with e:
diff --git a/lib/spack/spack/test/cmd/help.py b/lib/spack/spack/test/cmd/help.py
index ed7f872275..99f1ac7ed3 100644
--- a/lib/spack/spack/test/cmd/help.py
+++ b/lib/spack/spack/test/cmd/help.py
@@ -11,7 +11,7 @@ from spack.main import SpackCommand
 @pytest.mark.xfail
 def test_reuse_after_help():
     """Test `spack help` can be called twice with the same SpackCommand."""
-    help_cmd = SpackCommand("help")
+    help_cmd = SpackCommand("help", subprocess=True)
     help_cmd()
 
     # This second invocation will somehow fail because the parser no
@@ -30,14 +30,14 @@ def test_reuse_after_help():
 
 def test_help():
     """Sanity check the help command to make sure it works."""
-    help_cmd = SpackCommand("help")
+    help_cmd = SpackCommand("help", subprocess=True)
     out = help_cmd()
     assert "These are common spack commands:" in out
 
 
 def test_help_all():
     """Test the spack help --all flag"""
-    help_cmd = SpackCommand("help")
+    help_cmd = SpackCommand("help", subprocess=True)
     out = help_cmd("--all")
     assert "Complete list of spack commands:" in out
 
diff --git a/lib/spack/spack/test/cmd/is_git_repo.py b/lib/spack/spack/test/cmd/is_git_repo.py
index e094476a2e..27fb15cd5a 100644
--- a/lib/spack/spack/test/cmd/is_git_repo.py
+++ b/lib/spack/spack/test/cmd/is_git_repo.py
@@ -10,7 +10,7 @@ import sys
 
 import pytest
 
-from llnl.util.filesystem import mkdirp
+from llnl.util.filesystem import mkdirp, working_dir
 
 import spack
 from spack.util.executable import which
@@ -40,28 +40,29 @@ pytestmark = pytest.mark.skipif(
 
 
 @pytest.fixture(scope="function")
-def git_tmp_worktree(tmpdir):
+def git_tmp_worktree(tmpdir, mock_git_version_info):
     """Create new worktree in a temporary folder and monkeypatch
     spack.paths.prefix to point to it.
     """
-    # TODO: This is fragile and should be high priority for
-    # follow up fixes. 27021
-    # Path length is occasionally too long on Windows
-    # the following reduces the path length to acceptable levels
-    if sys.platform == "win32":
-        long_pth = str(tmpdir).split(os.path.sep)
-        tmp_worktree = os.path.sep.join(long_pth[:-1])
-    else:
-        tmp_worktree = str(tmpdir)
-    worktree_root = os.path.sep.join([tmp_worktree, "wrktree"])
+    with working_dir(mock_git_version_info[0]):
+        # TODO: This is fragile and should be high priority for
+        # follow up fixes. 27021
+        # Path length is occasionally too long on Windows
+        # the following reduces the path length to acceptable levels
+        if sys.platform == "win32":
+            long_pth = str(tmpdir).split(os.path.sep)
+            tmp_worktree = os.path.sep.join(long_pth[:-1])
+        else:
+            tmp_worktree = str(tmpdir)
+        worktree_root = os.path.sep.join([tmp_worktree, "wrktree"])
 
-    mkdirp(worktree_root)
+        mkdirp(worktree_root)
 
-    git("worktree", "add", "--detach", worktree_root, "HEAD")
+        git("worktree", "add", "--detach", worktree_root, "HEAD")
 
-    yield worktree_root
+        yield worktree_root
 
-    git("worktree", "remove", "--force", worktree_root)
+        git("worktree", "remove", "--force", worktree_root)
 
 
 def test_is_git_repo_in_worktree(git_tmp_worktree):
diff --git a/lib/spack/spack/test/cmd/module.py b/lib/spack/spack/test/cmd/module.py
index 1af511b85c..5ec99b0c64 100644
--- a/lib/spack/spack/test/cmd/module.py
+++ b/lib/spack/spack/test/cmd/module.py
@@ -210,7 +210,7 @@ def test_setdefault_command(mutable_database, mutable_config):
     for k in preferred, other_spec:
         assert os.path.exists(writers[k].layout.filename)
     assert os.path.exists(link_name) and os.path.islink(link_name)
-    assert os.path.realpath(link_name) == writers[other_spec].layout.filename
+    assert os.path.realpath(link_name) == os.path.realpath(writers[other_spec].layout.filename)
 
     # Reset the default to be the preferred spec
     module("lmod", "setdefault", preferred)
@@ -219,4 +219,4 @@ def test_setdefault_command(mutable_database, mutable_config):
     for k in preferred, other_spec:
         assert os.path.exists(writers[k].layout.filename)
     assert os.path.exists(link_name) and os.path.islink(link_name)
-    assert os.path.realpath(link_name) == writers[preferred].layout.filename
+    assert os.path.realpath(link_name) == os.path.realpath(writers[preferred].layout.filename)
diff --git a/lib/spack/spack/test/cmd/style.py b/lib/spack/spack/test/cmd/style.py
index dec795ca99..179973af52 100644
--- a/lib/spack/spack/test/cmd/style.py
+++ b/lib/spack/spack/test/cmd/style.py
@@ -46,22 +46,22 @@ skip_old_python = pytest.mark.skipif(
 
 
 @pytest.fixture(scope="function")
-def flake8_package():
+def flake8_package(tmpdir):
     """Style only checks files that have been modified. This fixture makes a small
     change to the ``flake8`` mock package, yields the filename, then undoes the
     change on cleanup.
     """
     repo = spack.repo.Repo(spack.paths.mock_packages_path)
     filename = repo.filename_for_package_name("flake8")
-    tmp = filename + ".tmp"
+    rel_path = os.path.dirname(os.path.relpath(filename, spack.paths.prefix))
+    tmp = tmpdir / rel_path / "flake8-ci-package.py"
+    tmp.ensure()
+    tmp = str(tmp)
 
-    try:
-        shutil.copy(filename, tmp)
-        package = FileFilter(filename)
-        package.filter("state = 'unmodified'", "state = 'modified'", string=True)
-        yield filename
-    finally:
-        shutil.move(tmp, filename)
+    shutil.copy(filename, tmp)
+    package = FileFilter(tmp)
+    package.filter("state = 'unmodified'", "state = 'modified'", string=True)
+    yield tmp
 
 
 @pytest.fixture
@@ -71,20 +71,17 @@ def flake8_package_with_errors(scope="function"):
     filename = repo.filename_for_package_name("flake8")
     tmp = filename + ".tmp"
 
-    try:
-        shutil.copy(filename, tmp)
-        package = FileFilter(filename)
+    shutil.copy(filename, tmp)
+    package = FileFilter(tmp)
 
-        # this is a black error (quote style and spacing before/after operator)
-        package.filter('state = "unmodified"', "state    =    'modified'", string=True)
+    # this is a black error (quote style and spacing before/after operator)
+    package.filter('state = "unmodified"', "state    =    'modified'", string=True)
 
-        # this is an isort error (orderign) and a flake8 error (unused import)
-        package.filter(
-            "from spack.package import *", "from spack.package import *\nimport os", string=True
-        )
-        yield filename
-    finally:
-        shutil.move(tmp, filename)
+    # this is an isort error (orderign) and a flake8 error (unused import)
+    package.filter(
+        "from spack.package import *", "from spack.package import *\nimport os", string=True
+    )
+    yield tmp
 
 
 def test_changed_files_from_git_rev_base(tmpdir, capfd):
@@ -125,7 +122,7 @@ def test_changed_no_base(tmpdir, capfd):
         assert "This repository does not have a 'foobar'" in err
 
 
-def test_changed_files_all_files(flake8_package):
+def test_changed_files_all_files():
     # it's hard to guarantee "all files", so do some sanity checks.
     files = set(
         [
@@ -139,13 +136,18 @@ def test_changed_files_all_files(flake8_package):
 
     # a builtin package
     zlib = spack.repo.path.get_pkg_class("zlib")
-    assert zlib.module.__file__ in files
+    zlib_file = zlib.module.__file__
+    if zlib_file.endswith("pyc"):
+        zlib_file = zlib_file[:-1]
+    assert zlib_file in files
 
     # a core spack file
     assert os.path.join(spack.paths.module_path, "spec.py") in files
 
     # a mock package
-    assert flake8_package in files
+    repo = spack.repo.Repo(spack.paths.mock_packages_path)
+    filename = repo.filename_for_package_name("flake8")
+    assert filename in files
 
     # this test
     assert __file__ in files
diff --git a/lib/spack/spack/test/cmd_extensions.py b/lib/spack/spack/test/cmd_extensions.py
index e68ac434fc..6947725afa 100644
--- a/lib/spack/spack/test/cmd_extensions.py
+++ b/lib/spack/spack/test/cmd_extensions.py
@@ -228,14 +228,17 @@ def test_missing_command():
     ],
     ids=["no_stem", "vacuous", "leading_hyphen", "basic_good", "trailing_slash", "hyphenated"],
 )
-def test_extension_naming(extension_path, expected_exception, config):
+def test_extension_naming(tmpdir, extension_path, expected_exception, config):
     """Ensure that we are correctly validating configured extension paths
     for conformity with the rules: the basename should match
     ``spack-<name>``; <name> may have embedded hyphens but not begin with one.
     """
-    with spack.config.override("config:extensions", [extension_path]):
-        with pytest.raises(expected_exception):
-            spack.cmd.get_module("no-such-command")
+    # NOTE: if the directory is a valid extension directory name the "vacuous" test will
+    # fail because it resolves to current working directory
+    with tmpdir.as_cwd():
+        with spack.config.override("config:extensions", [extension_path]):
+            with pytest.raises(expected_exception):
+                spack.cmd.get_module("no-such-command")
 
 
 def test_missing_command_function(extension_creator, capsys):
diff --git a/lib/spack/spack/util/debug.py b/lib/spack/spack/util/debug.py
index eb3b59a4fc..bdeee2df6c 100644
--- a/lib/spack/spack/util/debug.py
+++ b/lib/spack/spack/util/debug.py
@@ -10,6 +10,7 @@ a stack trace and drops the user into an interpreter.
 
 """
 import code
+import io
 import os
 import pdb
 import signal
@@ -53,7 +54,10 @@ class ForkablePdb(pdb.Pdb):
     the run of Spack.install, or any where else Spack spawns a child process.
     """
 
-    _original_stdin_fd = sys.stdin.fileno()
+    try:
+        _original_stdin_fd = sys.stdin.fileno()
+    except io.UnsupportedOperation:
+        _original_stdin_fd = None
     _original_stdin = None
 
     def __init__(self, stdout_fd=None, stderr_fd=None):
-- 
cgit v1.2.3-60-g2f50