openblas: derives from MakefilePackage (#2488)

* MakefilePackage: changed build_args and install_args for consistency with #2464
openblas: derives from MakefilePackage

* MakefilePackage: changed default edit behavior

lib/spack/spack/build_systems/makefile.py

@@ -25,6 +25,7 @@
 
 import inspect
 
+import llnl.util.tty as tty
 from llnl.util.filesystem import working_dir
 from spack.package import PackageBase
 
@@ -45,33 +46,26 @@ class MakefilePackage(PackageBase):
     # build-system class we are using
     build_system_class = 'MakefilePackage'
 
+    build_targets = []
+    install_targets = ['install']
+
     def build_directory(self):
         """Directory where the main Makefile is located"""
         return self.stage.source_path
 
-    def build_args(self):
-        """List of arguments that should be passed to make at build time"""
-        return []
-
-    def install_args(self):
-        """List of arguments that should be passed to make at install time"""
-        return []
-
     def edit(self, spec, prefix):
         """This phase cannot be defaulted for obvious reasons..."""
-        raise NotImplementedError('\'edit\' function not implemented')
+        tty.msg('Using default implementation: skipping edit phase.')
 
     def build(self, spec, prefix):
         """Default build phase : call make passing build_args"""
-        args = self.build_args()
         with working_dir(self.build_directory()):
-            inspect.getmodule(self).make(*args)
+            inspect.getmodule(self).make(*self.build_targets)
 
     def install(self, spec, prefix):
         """Default install phase : call make passing install_args"""
-        args = self.install_args() + ['install']
         with working_dir(self.build_directory()):
-            inspect.getmodule(self).make(*args)
+            inspect.getmodule(self).make(*self.install_targets)
 
     # Check that self.prefix is there after installation
     PackageBase.sanity_check('install')(PackageBase.sanity_check_prefix)

var/spack/repos/builtin/packages/astyle/package.py

@@ -49,5 +49,6 @@ def edit(self, spec, prefix):
         if sys.platform == 'darwin':
             filter_file(r'^INSTALL=.*', 'INSTALL=install', makefile)
 
-    def install_args(self):
-        return ['prefix={0}'.format(prefix)]
+    @property
+    def install_targets(self):
+        return ['install', 'prefix={0}'.format(self.prefix)]

var/spack/repos/builtin/packages/openblas/package.py

@@ -27,10 +27,10 @@
 import os
 
 
-class Openblas(Package):
+class Openblas(MakefilePackage):
     """OpenBLAS: An optimized BLAS library"""
-    homepage = "http://www.openblas.net"
-    url = "http://github.com/xianyi/OpenBLAS/archive/v0.2.15.tar.gz"
+    homepage = 'http://www.openblas.net'
+    url = 'http://github.com/xianyi/OpenBLAS/archive/v0.2.15.tar.gz'
 
     version('0.2.19', '28c998054fd377279741c6f0b9ea7941')
     version('0.2.18', '805e7f660877d588ea7e3792cda2ee65')
@@ -38,12 +38,13 @@ class Openblas(Package):
     version('0.2.16', 'fef46ab92463bdbb1479dcec594ef6dc')
     version('0.2.15', 'b1190f3d3471685f17cfd1ec1d252ac9')
 
-    variant('shared', default=True,
-            description="Build shared libraries as well as static libs.")
-    variant('openmp', default=False,
-            description="Enable OpenMP support.")
-    variant('fpic',   default=True,
-            description="Build position independent code")
+    variant(
+        'shared',
+        default=True,
+        description='Build shared libraries as well as static libs.'
+    )
+    variant('openmp', default=False, description="Enable OpenMP support.")
+    variant('pic', default=True, description='Build position independent code')
 
     # virtual dependency
     provides('blas')
@@ -65,61 +66,79 @@ def blas_libs(self):
     def lapack_libs(self):
         return self.blas_libs
 
-    def install(self, spec, prefix):
+    @MakefilePackage.precondition('edit')
+    def check_compilers(self):
         # As of 06/2016 there is no mechanism to specify that packages which
         # depends on Blas/Lapack need C or/and Fortran symbols. For now
         # require both.
         if self.compiler.f77 is None:
-            raise InstallError('OpenBLAS requires both C and Fortran ',
-                               'compilers!')
+            raise InstallError(
+                'OpenBLAS requires both C and Fortran compilers!'
+            )
+        # Add support for OpenMP
+        if '+openmp' in self.spec and self.spec.satisfies('%clang'):
+            # Openblas (as of 0.2.18) hardcoded that OpenMP cannot
+            # be used with any (!) compiler named clang, bummer.
+            raise InstallError(
+                'OpenBLAS does not support OpenMP with clang!'
+            )
 
+    @property
+    def make_defs(self):
         # Configure fails to pick up fortran from FC=/abs/path/to/f77, but
         # works fine with FC=/abs/path/to/gfortran.
         # When mixing compilers make sure that
         # $SPACK_ROOT/lib/spack/env/<compiler> have symlinks with reasonable
         # names and hack them inside lib/spack/spack/compilers/<compiler>.py
-        make_defs = ['CC=%s' % spack_cc,
-                     'FC=%s' % spack_f77,
-                     'MAKE_NO_J=1']
-
-        make_targets = ['libs', 'netlib']
-
-        # Build shared if variant is set.
-        if '+shared' in spec:
-            make_targets += ['shared']
-        else:
-            if '+fpic' in spec:
-                make_defs.extend(['CFLAGS=-fPIC', 'FFLAGS=-fPIC'])
+        make_defs = [
+            'CC={0}'.format(spack_cc),
+            'FC={0}'.format(spack_f77),
+            'MAKE_NO_J=1'
+        ]
+        if '~shared' in self.spec:
+            if '+pic' in self.spec:
+                make_defs.extend([
+                    'CFLAGS={0}'.format(self.compiler.pic_flag),
+                    'FFLAGS={0}'.format(self.compiler.pic_flag)
+                ])
             make_defs += ['NO_SHARED=1']
-
         # fix missing _dggsvd_ and _sggsvd_
-        if spec.satisfies('@0.2.16'):
+        if self.spec.satisfies('@0.2.16'):
             make_defs += ['BUILD_LAPACK_DEPRECATED=1']
-
         # Add support for OpenMP
-        if '+openmp' in spec:
-            # Openblas (as of 0.2.18) hardcoded that OpenMP cannot
-            # be used with any (!) compiler named clang, bummer.
-            if spec.satisfies('%clang'):
-                raise InstallError('OpenBLAS does not support ',
-                                   'OpenMP with clang!')
-
+        if '+openmp' in self.spec:
             make_defs += ['USE_OPENMP=1']
 
-        make_args = make_defs + make_targets
-        make(*make_args)
+        return make_defs
 
-        make("tests", *make_defs)
+    @property
+    def build_targets(self):
+        targets = ['libs', 'netlib']
 
-        # no quotes around prefix (spack doesn't use a shell)
-        make('install', "PREFIX=%s" % prefix, *make_defs)
+        # Build shared if variant is set.
+        if '+shared' in self.spec:
+            targets += ['shared']
 
+        return self.make_defs + targets
+
+    @MakefilePackage.sanity_check('build')
+    def check_build(self):
+        make('tests', *self.make_defs)
+
+    @property
+    def install_targets(self):
+        make_args = [
+            'install',
+            'PREFIX={0}'.format(self.prefix),
+        ]
+        return make_args + self.make_defs
+
+    @MakefilePackage.sanity_check('install')
+    def check_install(self):
+        spec = self.spec
         # Openblas may pass its own test but still fail to compile Lapack
         # symbols. To make sure we get working Blas and Lapack, do a small
         # test.
-        self.check_install(spec)
-
-    def check_install(self, spec):
         source_file = join_path(os.path.dirname(self.module.__file__),
                                 'test_cblas_dgemm.c')
         blessed_file = join_path(os.path.dirname(self.module.__file__),