Even better Makefile target parsing (#8819)

#8223 replaced regex-based makefile target parsing with an invocation of
"make -q". #8818 discovered that "make -q" can result in an error for some
packages.

Also, the "make -q" strategy relied on interpreting the error code, which only
worked for GNU Make and not BSD Make (which was deemed acceptable at
the time). As an added bonus, this implementation ignores the exit code and
instead parses STDERR for any indications that the target does not exist; this
works for both GNU Make and BSD Make.

#8223 also updated ninja target detection to use "ninja -t targets". This does
not change that behavior but makes it more-explicit with "ninja -t targets all"

This also adds tests for detection of "make" and "ninja" targets.

30 files changed, 301 insertions, 34 deletions

diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index d3a716f552..b333c7e0c2 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -1139,7 +1139,15 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
         packages_dir = spack.store.layout.build_packages_path(self.spec)
         dump_packages(self.spec, packages_dir)
 
-    def _if_make_target_execute(self, target):
+    def _has_make_target(self, target):
+        """Checks to see if 'target' is a valid target in a Makefile.
+
+        Parameters:
+            target (str): the target to check for
+
+        Returns:
+            bool: True if 'target' is found, else False
+        """
         make = inspect.getmodule(self).make
 
         # Check if we have a Makefile
@@ -1148,49 +1156,68 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
                 break
         else:
             tty.msg('No Makefile found in the build directory')
-            return
+            return False
 
-        # Check if 'target' is a valid target
-        #
-        # -q, --question
-        #       ``Question mode''. Do not run any commands, or print anything;
-        #       just return an exit status that is zero if the specified
-        #       targets are already up to date, nonzero otherwise.
+        # Check if 'target' is a valid target.
         #
-        # https://www.gnu.org/software/make/manual/html_node/Options-Summary.html
+        # `make -n target` performs a "dry run". It prints the commands that
+        # would be run but doesn't actually run them. If the target does not
+        # exist, you will see one of the following error messages:
         #
-        # The exit status of make is always one of three values:
+        # GNU Make:
+        #     make: *** No rule to make target `test'.  Stop.
         #
-        # 0     The exit status is zero if make is successful.
-        #
-        # 2     The exit status is two if make encounters any errors.
-        #       It will print messages describing the particular errors.
-        #
-        # 1     The exit status is one if you use the '-q' flag and make
-        #       determines that some target is not already up to date.
-        #
-        # https://www.gnu.org/software/make/manual/html_node/Running.html
-        #
-        # NOTE: This only works for GNU Make, not NetBSD Make.
-        make('-q', target, fail_on_error=False)
-        if make.returncode == 2:
-            tty.msg("Target '" + target + "' not found in " + makefile)
-            return
+        # BSD Make:
+        #     make: don't know how to make test. Stop
+        missing_target_msgs = [
+            "No rule to make target `{0}'.  Stop.",
+            "don't know how to make {0}. Stop",
+        ]
+
+        kwargs = {
+            'fail_on_error': False,
+            'output': os.devnull,
+            'error': str,
+        }
+
+        stderr = make('-n', target, **kwargs)
+
+        for missing_target_msg in missing_target_msgs:
+            if missing_target_msg.format(target) in stderr:
+                tty.msg("Target '" + target + "' not found in " + makefile)
+                return False
 
-        # Execute target
-        make(target)
+        return True
 
-    def _if_ninja_target_execute(self, target):
+    def _if_make_target_execute(self, target):
+        """Runs ``make target`` if 'target' is a valid target in the Makefile.
+
+        Parameters:
+            target (str): the target to potentially execute
+        """
+        if self._has_make_target(target):
+            # Execute target
+            inspect.getmodule(self).make(target)
+
+    def _has_ninja_target(self, target):
+        """Checks to see if 'target' is a valid target in a Ninja build script.
+
+        Parameters:
+            target (str): the target to check for
+
+        Returns:
+            bool: True if 'target' is found, else False
+        """
         ninja = inspect.getmodule(self).ninja
 
         # Check if we have a Ninja build script
         if not os.path.exists('build.ninja'):
             tty.msg('No Ninja build script found in the build directory')
-            return
+            return False
 
         # Get a list of all targets in the Ninja build script
         # https://ninja-build.org/manual.html#_extra_tools
-        all_targets = ninja('-t', 'targets', output=str).split('\n')
+        all_targets = ninja('-t', 'targets', 'all', output=str).split('\n')
 
         # Check if 'target' is a valid target
         matches = [line for line in all_targets
@@ -1198,10 +1225,20 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
 
         if not matches:
             tty.msg("Target '" + target + "' not found in build.ninja")
-            return
+            return False
+
+        return True
+
+    def _if_ninja_target_execute(self, target):
+        """Runs ``ninja target`` if 'target' is a valid target in the Ninja
+        build script.
 
-        # Execute target
-        ninja(target)
+        Parameters:
+            target (str): the target to potentially execute
+        """
+        if self._has_ninja_target(target):
+            # Execute target
+            inspect.getmodule(self).ninja(target)
 
     def _get_needed_resources(self):
         resources = []
diff --git a/lib/spack/spack/test/build_systems.py b/lib/spack/spack/test/build_systems.py
index cac0711b48..8915b813cb 100644
--- a/lib/spack/spack/test/build_systems.py
+++ b/lib/spack/spack/test/build_systems.py
@@ -22,11 +22,101 @@
 # License along with this program; if not, write to the Free Software
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 ##############################################################################
+import glob
+import os
 import pytest
 
 import spack.repo
-from spack.build_environment import get_std_cmake_args
+from llnl.util.filesystem import working_dir
+from spack.build_environment import get_std_cmake_args, setup_package
 from spack.spec import Spec
+from spack.util.executable import which
+
+
+DATA_PATH = os.path.join(spack.paths.test_path, 'data')
+
+
+@pytest.mark.parametrize(
+    'directory',
+    glob.iglob(os.path.join(DATA_PATH, 'make', 'affirmative', '*'))
+)
+def test_affirmative_make_check(directory, config, mock_packages):
+    """Tests that Spack correctly detects targets in a Makefile."""
+
+    # Get a fake package
+    s = Spec('mpich')
+    s.concretize()
+    pkg = spack.repo.get(s)
+    setup_package(pkg, False)
+
+    with working_dir(directory):
+        assert pkg._has_make_target('check')
+
+        pkg._if_make_target_execute('check')
+
+
+@pytest.mark.parametrize(
+    'directory',
+    glob.iglob(os.path.join(DATA_PATH, 'make', 'negative', '*'))
+)
+def test_negative_make_check(directory, config, mock_packages):
+    """Tests that Spack correctly ignores false positives in a Makefile."""
+
+    # Get a fake package
+    s = Spec('mpich')
+    s.concretize()
+    pkg = spack.repo.get(s)
+    setup_package(pkg, False)
+
+    with working_dir(directory):
+        assert not pkg._has_make_target('check')
+
+        pkg._if_make_target_execute('check')
+
+
+@pytest.mark.skipif(not which('ninja'), reason='ninja is not installed')
+@pytest.mark.parametrize(
+    'directory',
+    glob.iglob(os.path.join(DATA_PATH, 'ninja', 'affirmative', '*'))
+)
+def test_affirmative_ninja_check(directory, config, mock_packages):
+    """Tests that Spack correctly detects targets in a Ninja build script."""
+
+    # Get a fake package
+    s = Spec('mpich')
+    s.concretize()
+    pkg = spack.repo.get(s)
+    setup_package(pkg, False)
+
+    with working_dir(directory):
+        assert pkg._has_ninja_target('check')
+
+        pkg._if_ninja_target_execute('check')
+
+        # Clean up Ninja files
+        for filename in glob.iglob('.ninja_*'):
+            os.remove(filename)
+
+
+@pytest.mark.skipif(not which('ninja'), reason='ninja is not installed')
+@pytest.mark.parametrize(
+    'directory',
+    glob.iglob(os.path.join(DATA_PATH, 'ninja', 'negative', '*'))
+)
+def test_negative_ninja_check(directory, config, mock_packages):
+    """Tests that Spack correctly ignores false positives in a Ninja
+    build script."""
+
+    # Get a fake package
+    s = Spec('mpich')
+    s.concretize()
+    pkg = spack.repo.get(s)
+    setup_package(pkg, False)
+
+    with working_dir(directory):
+        assert not pkg._has_ninja_target('check')
+
+        pkg._if_ninja_target_execute('check')
 
 
 def test_cmake_std_args(config, mock_packages):
diff --git a/lib/spack/spack/test/data/make/affirmative/capital_makefile/Makefile b/lib/spack/spack/test/data/make/affirmative/capital_makefile/Makefile
new file mode 100644
index 0000000000..a580687aa0
--- /dev/null
+++ b/lib/spack/spack/test/data/make/affirmative/capital_makefile/Makefile
@@ -0,0 +1,3 @@
+# Tests that Spack checks for Makefile
+
+check:
diff --git a/lib/spack/spack/test/data/make/affirmative/check_test/Makefile b/lib/spack/spack/test/data/make/affirmative/check_test/Makefile
new file mode 100644
index 0000000000..bbd931b84c
--- /dev/null
+++ b/lib/spack/spack/test/data/make/affirmative/check_test/Makefile
@@ -0,0 +1,3 @@
+# Tests that Spack detects target when it is the first of two targets
+
+check test:
diff --git a/lib/spack/spack/test/data/make/affirmative/expansion/Makefile b/lib/spack/spack/test/data/make/affirmative/expansion/Makefile
new file mode 100644
index 0000000000..e73101f01a
--- /dev/null
+++ b/lib/spack/spack/test/data/make/affirmative/expansion/Makefile
@@ -0,0 +1,5 @@
+# Tests that Spack can handle variable expansion targets
+
+TARGETS = check
+
+$(TARGETS):
diff --git a/lib/spack/spack/test/data/make/affirmative/gnu_makefile/GNUmakefile b/lib/spack/spack/test/data/make/affirmative/gnu_makefile/GNUmakefile
new file mode 100644
index 0000000000..77ea4d0972
--- /dev/null
+++ b/lib/spack/spack/test/data/make/affirmative/gnu_makefile/GNUmakefile
@@ -0,0 +1,3 @@
+# Tests that Spack checks for GNUmakefile
+
+check:
diff --git a/lib/spack/spack/test/data/make/affirmative/include/Makefile b/lib/spack/spack/test/data/make/affirmative/include/Makefile
new file mode 100644
index 0000000000..f24ab957cb
--- /dev/null
+++ b/lib/spack/spack/test/data/make/affirmative/include/Makefile
@@ -0,0 +1,3 @@
+# Tests that Spack detects targets in include files
+
+include make.mk
diff --git a/lib/spack/spack/test/data/make/affirmative/include/make.mk b/lib/spack/spack/test/data/make/affirmative/include/make.mk
new file mode 100644
index 0000000000..76e4478aae
--- /dev/null
+++ b/lib/spack/spack/test/data/make/affirmative/include/make.mk
@@ -0,0 +1 @@
+check:
diff --git a/lib/spack/spack/test/data/make/affirmative/lowercase_makefile/makefile b/lib/spack/spack/test/data/make/affirmative/lowercase_makefile/makefile
new file mode 100644
index 0000000000..942f8ab96e
--- /dev/null
+++ b/lib/spack/spack/test/data/make/affirmative/lowercase_makefile/makefile
@@ -0,0 +1,3 @@
+# Tests that Spack checks for makefile
+
+check:
diff --git a/lib/spack/spack/test/data/make/affirmative/prerequisites/Makefile b/lib/spack/spack/test/data/make/affirmative/prerequisites/Makefile
new file mode 100644
index 0000000000..22b42f3f83
--- /dev/null
+++ b/lib/spack/spack/test/data/make/affirmative/prerequisites/Makefile
@@ -0,0 +1,5 @@
+# Tests that Spack detects a target even if it is followed by prerequisites
+
+check: check-recursive
+
+check-recursive:
diff --git a/lib/spack/spack/test/data/make/affirmative/spaces/Makefile b/lib/spack/spack/test/data/make/affirmative/spaces/Makefile
new file mode 100644
index 0000000000..c9b5d4d920
--- /dev/null
+++ b/lib/spack/spack/test/data/make/affirmative/spaces/Makefile
@@ -0,0 +1,3 @@
+# Tests that Spack allows spaces following the target name
+
+check    :
diff --git a/lib/spack/spack/test/data/make/affirmative/test_check/Makefile b/lib/spack/spack/test/data/make/affirmative/test_check/Makefile
new file mode 100644
index 0000000000..5924d7d702
--- /dev/null
+++ b/lib/spack/spack/test/data/make/affirmative/test_check/Makefile
@@ -0,0 +1,3 @@
+# Tests that Spack detects target when it is the second of two targets
+
+test check:
diff --git a/lib/spack/spack/test/data/make/affirmative/three_targets/Makefile b/lib/spack/spack/test/data/make/affirmative/three_targets/Makefile
new file mode 100644
index 0000000000..96d7eff3af
--- /dev/null
+++ b/lib/spack/spack/test/data/make/affirmative/three_targets/Makefile
@@ -0,0 +1,3 @@
+# Tests that Spack detects a target if it is in the middle of a list
+
+foo check bar:
diff --git a/lib/spack/spack/test/data/make/negative/no_makefile/readme.txt b/lib/spack/spack/test/data/make/negative/no_makefile/readme.txt
new file mode 100644
index 0000000000..836ad0edb6
--- /dev/null
+++ b/lib/spack/spack/test/data/make/negative/no_makefile/readme.txt
@@ -0,0 +1,3 @@
+# Tests that Spack ignores directories without a Makefile
+
+check:
diff --git a/lib/spack/spack/test/data/make/negative/partial_match/Makefile b/lib/spack/spack/test/data/make/negative/partial_match/Makefile
new file mode 100644
index 0000000000..ea315731eb
--- /dev/null
+++ b/lib/spack/spack/test/data/make/negative/partial_match/Makefile
@@ -0,0 +1,11 @@
+# Tests that Spack ignores targets that contain a partial match
+
+checkinstall:
+
+installcheck:
+
+foo-check-bar:
+
+foo_check_bar:
+
+foo/check/bar:
diff --git a/lib/spack/spack/test/data/make/negative/variable/Makefile b/lib/spack/spack/test/data/make/negative/variable/Makefile
new file mode 100644
index 0000000000..586aea18dc
--- /dev/null
+++ b/lib/spack/spack/test/data/make/negative/variable/Makefile
@@ -0,0 +1,5 @@
+# Tests that Spack ignores variable definitions
+
+check = FOO
+
+check := BAR
diff --git a/lib/spack/spack/test/data/ninja/.gitignore b/lib/spack/spack/test/data/ninja/.gitignore
new file mode 100644
index 0000000000..50e58f24cc
--- /dev/null
+++ b/lib/spack/spack/test/data/ninja/.gitignore
@@ -0,0 +1,2 @@
+.ninja_deps
+.ninja_log
diff --git a/lib/spack/spack/test/data/ninja/affirmative/check_test/build.ninja b/lib/spack/spack/test/data/ninja/affirmative/check_test/build.ninja
new file mode 100644
index 0000000000..e3af305dbe
--- /dev/null
+++ b/lib/spack/spack/test/data/ninja/affirmative/check_test/build.ninja
@@ -0,0 +1,6 @@
+# Tests that Spack detects target when it is the first of two targets
+
+rule cc
+  command = true
+
+build check test: cc
diff --git a/lib/spack/spack/test/data/ninja/affirmative/include/build.ninja b/lib/spack/spack/test/data/ninja/affirmative/include/build.ninja
new file mode 100644
index 0000000000..c9ce4e61a6
--- /dev/null
+++ b/lib/spack/spack/test/data/ninja/affirmative/include/build.ninja
@@ -0,0 +1,3 @@
+# Tests that Spack can handle targets in include files
+
+include include.ninja
diff --git a/lib/spack/spack/test/data/ninja/affirmative/include/include.ninja b/lib/spack/spack/test/data/ninja/affirmative/include/include.ninja
new file mode 100644
index 0000000000..8a0d0f7c1d
--- /dev/null
+++ b/lib/spack/spack/test/data/ninja/affirmative/include/include.ninja
@@ -0,0 +1,4 @@
+rule cc
+  command = true
+
+build check: cc
diff --git a/lib/spack/spack/test/data/ninja/affirmative/simple/build.ninja b/lib/spack/spack/test/data/ninja/affirmative/simple/build.ninja
new file mode 100644
index 0000000000..f002938b10
--- /dev/null
+++ b/lib/spack/spack/test/data/ninja/affirmative/simple/build.ninja
@@ -0,0 +1,6 @@
+# Tests that Spack can handle a simple Ninja build script
+
+rule cc
+  command = true
+
+build check: cc
diff --git a/lib/spack/spack/test/data/ninja/affirmative/spaces/build.ninja b/lib/spack/spack/test/data/ninja/affirmative/spaces/build.ninja
new file mode 100644
index 0000000000..dd59a9932f
--- /dev/null
+++ b/lib/spack/spack/test/data/ninja/affirmative/spaces/build.ninja
@@ -0,0 +1,6 @@
+# Tests that Spack allows spaces following the target name
+
+rule cc
+  command = true
+
+build check   : cc
diff --git a/lib/spack/spack/test/data/ninja/affirmative/subninja/build.ninja b/lib/spack/spack/test/data/ninja/affirmative/subninja/build.ninja
new file mode 100644
index 0000000000..19ef7e3509
--- /dev/null
+++ b/lib/spack/spack/test/data/ninja/affirmative/subninja/build.ninja
@@ -0,0 +1,3 @@
+# Tests that Spack can handle targets in subninja files
+
+subninja subninja.ninja
diff --git a/lib/spack/spack/test/data/ninja/affirmative/subninja/subninja.ninja b/lib/spack/spack/test/data/ninja/affirmative/subninja/subninja.ninja
new file mode 100644
index 0000000000..8a0d0f7c1d
--- /dev/null
+++ b/lib/spack/spack/test/data/ninja/affirmative/subninja/subninja.ninja
@@ -0,0 +1,4 @@
+rule cc
+  command = true
+
+build check: cc
diff --git a/lib/spack/spack/test/data/ninja/affirmative/test_check/build.ninja b/lib/spack/spack/test/data/ninja/affirmative/test_check/build.ninja
new file mode 100644
index 0000000000..6a3cd0b0ff
--- /dev/null
+++ b/lib/spack/spack/test/data/ninja/affirmative/test_check/build.ninja
@@ -0,0 +1,6 @@
+# Tests that Spack detects target when it is the second of two targets
+
+rule cc
+  command = true
+
+build test check: cc
diff --git a/lib/spack/spack/test/data/ninja/affirmative/three_targets/build.ninja b/lib/spack/spack/test/data/ninja/affirmative/three_targets/build.ninja
new file mode 100644
index 0000000000..bf9e14ed0b
--- /dev/null
+++ b/lib/spack/spack/test/data/ninja/affirmative/three_targets/build.ninja
@@ -0,0 +1,6 @@
+# Tests that Spack detects a target if it is in the middle of a list
+
+rule cc
+  command = true
+
+build foo check bar: cc
diff --git a/lib/spack/spack/test/data/ninja/negative/no_ninja/readme.txt b/lib/spack/spack/test/data/ninja/negative/no_ninja/readme.txt
new file mode 100644
index 0000000000..0a85aa7af8
--- /dev/null
+++ b/lib/spack/spack/test/data/ninja/negative/no_ninja/readme.txt
@@ -0,0 +1,8 @@
+# Tests that Spack ignores directories without a Ninja build script
+
+cflags = -Wall
+
+rule cc
+  command = gcc $cflags -c $in -o $out
+
+build check: cc foo.c
diff --git a/lib/spack/spack/test/data/ninja/negative/partial_match/build.ninja b/lib/spack/spack/test/data/ninja/negative/partial_match/build.ninja
new file mode 100644
index 0000000000..12efb5839a
--- /dev/null
+++ b/lib/spack/spack/test/data/ninja/negative/partial_match/build.ninja
@@ -0,0 +1,16 @@
+# Tests that Spack ignores targets that contain a partial match
+
+cflags = -Wall
+
+rule cc
+  command = gcc $cflags -c $in -o $out
+
+build installcheck: cc foo.c
+
+build checkinstall: cc foo.c
+
+build foo-check-bar: cc foo.c
+
+build foo_check_bar: cc foo.c
+
+build foo/check/bar: cc foo.c
diff --git a/lib/spack/spack/test/data/ninja/negative/rule/build.ninja b/lib/spack/spack/test/data/ninja/negative/rule/build.ninja
new file mode 100644
index 0000000000..2164bda6d9
--- /dev/null
+++ b/lib/spack/spack/test/data/ninja/negative/rule/build.ninja
@@ -0,0 +1,8 @@
+# Tests that Spack ignores rule names
+
+cflags = -Wall
+
+rule check
+  command = gcc $cflags -c $in -o $out
+
+build foo: check foo.c
diff --git a/lib/spack/spack/test/data/ninja/negative/variable/build.ninja b/lib/spack/spack/test/data/ninja/negative/variable/build.ninja
new file mode 100644
index 0000000000..73aafbf5bd
--- /dev/null
+++ b/lib/spack/spack/test/data/ninja/negative/variable/build.ninja
@@ -0,0 +1,8 @@
+# Tests that Spack ignores variable definitions
+
+check = -Wall
+
+rule cc
+  command = gcc $check -c $in -o $out
+
+build foo: cc foo.c