Repository: scikit-build/scikit-build

Issue:
Improve cmaker exception
When there is a problem building python module, report "human-friendly" error

Discussion:

Patch:
diff --git a/skbuild/cmaker.py b/skbuild/cmaker.py
index 9030c1e..2c7d7f5 100644
--- a/skbuild/cmaker.py
+++ b/skbuild/cmaker.py
@@ -9,6 +9,8 @@ import shlex
 import sys
 import sysconfig
 
+from subprocess import CalledProcessError
+
 from .platform_specifics import get_platform
 from .exceptions import SKBuildError
 
@@ -62,10 +64,11 @@ def _touch_init(folder):
 class CMaker(object):
 
     def __init__(self, **defines):
-        if platform.system() != 'Windows':
-            rtn = subprocess.call(['which', 'cmake'])
-            if rtn != 0:
-                sys.exit('CMake is not installed, aborting build.')
+        # verify that CMake is installed
+        try:
+            subprocess.check_call(['cmake', '--version'])
+        except (OSError, CalledProcessError):
+            raise SKBuildError('CMake is not installed, aborting build.')
 
         self.platform = get_platform()
 
@@ -93,8 +96,9 @@ class CMaker(object):
         generator_id = self.platform.get_best_generator(generator_id)
 
         if generator_id is None:
-            sys.exit("Could not get working generator for your system."
-                     "  Aborting build.")
+            raise SKBuildError(
+                "Could not get working generator for your system."
+                "  Aborting build.")
 
         if not os.path.exists(CMAKE_BUILD_DIR):
             os.makedirs(CMAKE_BUILD_DIR)
@@ -137,11 +141,20 @@ class CMaker(object):
 
         # changes dir to cmake_build and calls cmake's configure step
         # to generate makefile
-        rtn = subprocess.check_call(cmd, cwd=CMAKE_BUILD_DIR)
+        rtn = subprocess.call(cmd, cwd=CMAKE_BUILD_DIR)
         if rtn != 0:
-            raise RuntimeError("Could not successfully configure "
-                               "your project. Please see CMake's "
-                               "output for more information.")
+            raise SKBuildError(
+                "An error occurred while configuring with CMake.\n"
+                "  Command:\n"
+                "    {}\n"
+                "  Source directory:\n"
+                "    {}\n"
+                "  Working directory:\n"
+                "    {}\n"
+                "Please see CMake's output for more information.".format(
+                    self._formatArgsForDisplay(cmd),
+                    os.path.abspath(cwd),
+                    os.path.abspath(CMAKE_BUILD_DIR)))
 
         CMaker.check_for_bad_installs()
 
@@ -335,7 +348,6 @@ class CMaker(object):
 
         if bad_installs:
             raise SKBuildError("\n".join((
-                "",
                 "  CMake-installed files must be within the project root.",
                 "    Project Root:",
                 "      " + install_dir,
@@ -349,7 +361,7 @@ class CMaker(object):
         """
         clargs, config = pop_arg('--config', clargs, config)
         if not os.path.exists(CMAKE_BUILD_DIR):
-            raise RuntimeError(("CMake build folder ({}) does not exist. "
+            raise SKBuildError(("CMake build folder ({}) does not exist. "
                                 "Did you forget to run configure before "
                                 "make?").format(CMAKE_BUILD_DIR))
 
@@ -361,8 +373,20 @@ class CMaker(object):
                    shlex.split(os.environ.get("SKBUILD_BUILD_OPTIONS", "")))
         )
 
-        rtn = subprocess.check_call(cmd, cwd=CMAKE_BUILD_DIR)
-        return rtn
+        rtn = subprocess.call(cmd, cwd=CMAKE_BUILD_DIR)
+        if rtn != 0:
+            raise SKBuildError(
+                "An error occurred while building with CMake.\n"
+                "  Command:\n"
+                "    {}\n"
+                "  Source directory:\n"
+                "    {}\n"
+                "  Working directory:\n"
+                "    {}\n"
+                "Please see CMake's output for more information.".format(
+                    self._formatArgsForDisplay(cmd),
+                    os.path.abspath(source_dir),
+                    os.path.abspath(CMAKE_BUILD_DIR)))
 
     def install(self):
         """Returns a list of tuples of (install location, file list) to install
@@ -377,3 +401,14 @@ class CMaker(object):
             return [_remove_cwd_prefix(path) for path in manifest]
 
         return []
+
+    @staticmethod
+    def _formatArgsForDisplay(args):
+        """Format a list of arguments appropriately for display. When formatting
+        a command and its arguments, the user should be able to execute the
+        command by copying and pasting the output directly into a shell.
+
+        Currently, the only formatting is naively surrounding each argument with
+        quotation marks.
+        """
+        return ' '.join("\"{}\"".format(arg) for arg in args)
diff --git a/skbuild/exceptions.py b/skbuild/exceptions.py
index 4a0e074..2b8f8b1 100644
--- a/skbuild/exceptions.py
+++ b/skbuild/exceptions.py
@@ -1,3 +1,6 @@
 
-class SKBuildError(Exception):
+class SKBuildError(RuntimeError):
+    """Exception raised when an error occurs while configuring or building a
+    project.
+    """
     pass
diff --git a/skbuild/setuptools_wrap.py b/skbuild/setuptools_wrap.py
index 0fbd86f..54efdb3 100644
--- a/skbuild/setuptools_wrap.py
+++ b/skbuild/setuptools_wrap.py
@@ -131,12 +131,56 @@ def setup(*args, **kw):
         reverse=True
     ))
 
-    cmkr = cmaker.CMaker()
-    cmkr.configure(cmake_args)
-    cmkr.make(make_args)
+    try:
+        cmkr = cmaker.CMaker()
+        cmkr.configure(cmake_args)
+        cmkr.make(make_args)
+    except SKBuildError as e:
+        import traceback
+        print("Traceback (most recent call last):")
+        traceback.print_tb(sys.exc_info()[2])
+        print()
+        sys.exit(e)
+
+    _classify_files(cmkr.install(), package_data, package_prefixes, py_modules,
+                    scripts, new_scripts, data_files)
+
+    kw['package_data'] = package_data
+    kw['package_dir'] = {
+        package: os.path.join(cmaker.CMAKE_INSTALL_DIR, prefix)
+        for prefix, package in package_prefixes
+    }
+
+    kw['py_modules'] = py_modules
+
+    kw['scripts'] = [
+        os.path.join(cmaker.CMAKE_INSTALL_DIR, script) if mask else script
+        for script, mask in new_scripts.items()
+    ]
+
+    kw['data_files'] = [
+        (parent_dir, list(file_set))
+        for parent_dir, file_set in data_files.items()
+    ]
+
+    # work around https://bugs.python.org/issue1011113
+    # (patches provided, but no updates since 2014)
+    cmdclass = kw.get('cmdclass', {})
+    cmdclass['build'] = cmdclass.get('build', build.build)
+    cmdclass['install'] = cmdclass.get('install', install.install)
+    cmdclass['clean'] = cmdclass.get('clean', clean.clean)
+    cmdclass['bdist'] = cmdclass.get('bdist', bdist.bdist)
+    cmdclass['bdist_wheel'] = cmdclass.get(
+        'bdist_wheel', bdist_wheel.bdist_wheel)
+    kw['cmdclass'] = cmdclass
+
+    return upstream_setup(*args, **kw)
 
+
+def _classify_files(install_paths, package_data, package_prefixes, py_modules,
+                    scripts, new_scripts, data_files):
     install_root = os.path.join(os.getcwd(), cmaker.CMAKE_INSTALL_DIR)
-    for path in cmkr.install():
+    for path in install_paths:
         found_package = False
         found_module = False
         found_script = False
@@ -204,34 +248,3 @@ def setup(*args, **kw):
             data_files[parent_dir] = file_set
         file_set.add(os.path.join(cmaker.CMAKE_INSTALL_DIR, path))
         del parent_dir, file_set
-
-    kw['package_data'] = package_data
-    kw['package_dir'] = {
-        package: os.path.join(cmaker.CMAKE_INSTALL_DIR, prefix)
-        for prefix, package in package_prefixes
-    }
-
-    kw['py_modules'] = py_modules
-
-    kw['scripts'] = [
-        os.path.join(cmaker.CMAKE_INSTALL_DIR, script) if mask else script
-        for script, mask in new_scripts.items()
-    ]
-
-    kw['data_files'] = [
-        (parent_dir, list(file_set))
-        for parent_dir, file_set in data_files.items()
-    ]
-
-    # work around https://bugs.python.org/issue1011113
-    # (patches provided, but no updates since 2014)
-    cmdclass = kw.get('cmdclass', {})
-    cmdclass['build'] = cmdclass.get('build', build.build)
-    cmdclass['install'] = cmdclass.get('install', install.install)
-    cmdclass['clean'] = cmdclass.get('clean', clean.clean)
-    cmdclass['bdist'] = cmdclass.get('bdist', bdist.bdist)
-    cmdclass['bdist_wheel'] = cmdclass.get(
-        'bdist_wheel', bdist_wheel.bdist_wheel)
-    kw['cmdclass'] = cmdclass
-
-    return upstream_setup(*args, **kw)


Test patch:
diff --git a/tests/test_outside_project_root.py b/tests/test_outside_project_root.py
index 9500a4d..d67baa4 100644
--- a/tests/test_outside_project_root.py
+++ b/tests/test_outside_project_root.py
@@ -5,7 +5,8 @@
 ----------------------------------
 
 Tries to build the `fail-outside-project-root` sample project.  Ensures that the
-attempt fails with an SKBuildError exception.
+attempt fails with a SystemExit exception that has an SKBuildError exception as
+its value.
 """
 
 from skbuild.exceptions import SKBuildError
@@ -23,10 +24,10 @@ def test_outside_project_root_fails():
         def should_fail():
             pass
 
-        exception_thrown = False
+        failed = False
         try:
             should_fail()
-        except SKBuildError:
-            exception_thrown = True
+        except SystemExit as e:
+            failed = isinstance(e.code, SKBuildError)
 
-        assert exception_thrown
+        assert failed