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.

.travis.yml

@@ -132,6 +132,7 @@ addons:
       - graphviz
       - gnupg2
       - cmake
+      - ninja-build
       - r-base
       - r-base-core
       - r-base-dev

lib/spack/spack/package.py

@@ -1139,7 +1139,15 @@ def do_fake_install(self):
         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 @@ def _if_make_target_execute(self, target):
                 break
         else:
             tty.msg('No Makefile found in the build directory')
-            return
+            return False
 
-        # Check if 'target' is a valid target
+        # 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.
+        # `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:
         #
-        # https://www.gnu.org/software/make/manual/html_node/Options-Summary.html
+        # GNU Make:
+        #     make: *** No rule to make target `test'.  Stop.
         #
-        # The exit status of make is always one of three values:
-        #
-        # 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",
+        ]
 
-        # Execute target
-        make(target)
+        kwargs = {
+            'fail_on_error': False,
+            'output': os.devnull,
+            'error': str,
+        }
 
-    def _if_ninja_target_execute(self, target):
+        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
+
+        return True
+
+    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 @@ def _if_ninja_target_execute(self, target):
 
         if not matches:
             tty.msg("Target '" + target + "' not found in build.ninja")
-            return
+            return False
 
-        # Execute target
-        ninja(target)
+        return True
+
+    def _if_ninja_target_execute(self, target):
+        """Runs ``ninja target`` if 'target' is a valid target in the Ninja
+        build script.
+
+        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 = []

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):

lib/spack/spack/test/data/make/affirmative/capital_makefile/Makefile

@@ -0,0 +1,3 @@
+# Tests that Spack checks for Makefile
+
+check:

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:

lib/spack/spack/test/data/make/affirmative/expansion/Makefile

@@ -0,0 +1,5 @@
+# Tests that Spack can handle variable expansion targets
+
+TARGETS = check
+
+$(TARGETS):

lib/spack/spack/test/data/make/affirmative/gnu_makefile/GNUmakefile

@@ -0,0 +1,3 @@
+# Tests that Spack checks for GNUmakefile
+
+check:

lib/spack/spack/test/data/make/affirmative/include/Makefile

@@ -0,0 +1,3 @@
+# Tests that Spack detects targets in include files
+
+include make.mk

lib/spack/spack/test/data/make/affirmative/include/make.mk

@@ -0,0 +1 @@
+check:

lib/spack/spack/test/data/make/affirmative/lowercase_makefile/makefile

@@ -0,0 +1,3 @@
+# Tests that Spack checks for makefile
+
+check:

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:

lib/spack/spack/test/data/make/affirmative/spaces/Makefile

@@ -0,0 +1,3 @@
+# Tests that Spack allows spaces following the target name
+
+check    :

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:

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:

lib/spack/spack/test/data/make/negative/no_makefile/readme.txt

@@ -0,0 +1,3 @@
+# Tests that Spack ignores directories without a Makefile
+
+check:

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:

lib/spack/spack/test/data/make/negative/variable/Makefile

@@ -0,0 +1,5 @@
+# Tests that Spack ignores variable definitions
+
+check = FOO
+
+check := BAR

lib/spack/spack/test/data/ninja/.gitignore

@@ -0,0 +1,2 @@
+.ninja_deps
+.ninja_log

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

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

lib/spack/spack/test/data/ninja/affirmative/include/include.ninja

@@ -0,0 +1,4 @@
+rule cc
+  command = true
+
+build check: cc

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

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

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

lib/spack/spack/test/data/ninja/affirmative/subninja/subninja.ninja

@@ -0,0 +1,4 @@
+rule cc
+  command = true
+
+build check: cc

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

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

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

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

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

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