Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for cached generators #301

Closed
wants to merge 15 commits into from
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ docs/_build
# IDE junk
.idea/*
*.swp
.vscode/*

# build output (testing)
skbuild/_cmake_test_compile/*
22 changes: 22 additions & 0 deletions skbuild/cmaker.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,28 @@ def __init__(self):

self.platform = get_platform()

def get_cached_generator_name(self):
"""Reads and returns the cached generator from the BUILD_DIR; returns None
if not found.
"""
try:
cmake_generator = 'CMAKE_GENERATOR:INTERNAL='
with open(os.path.join(CMAKE_BUILD_DIR, 'CMakeCache.txt')) as fp:
for line in fp:
if line.startswith(cmake_generator):
return line[len(cmake_generator):].strip()
except (OSError, IOError):
pass

return None

def get_cached_env(self):
generator_name = self.get_cached_generator_name()
if generator_name is not None:
return self.platform.get_generator(generator_name).env

return None

def configure(self, clargs=(), generator_name=None,
cmake_source_dir='.', cmake_install_dir='',
languages=('C', 'CXX'), cleanup=True):
Expand Down
1 change: 1 addition & 0 deletions skbuild/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,4 @@
CMAKE_BUILD_DIR = os.path.join(SKBUILD_DIR, "cmake-build")
CMAKE_INSTALL_DIR = os.path.join(SKBUILD_DIR, "cmake-install")
SETUPTOOLS_INSTALL_DIR = os.path.join(SKBUILD_DIR, "setuptools")
CMAKE_ARGUMENTS_FILE = os.path.join(CMAKE_BUILD_DIR, "CMakeArguments.json")
10 changes: 10 additions & 0 deletions skbuild/platform_specifics/abstract.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,16 @@ def get_cmake_exe_path(self):
"""
return "cmake"

def get_generator(self, generator_name):
"""Loop over generators and return the first that matches the given
name.
"""
for default_generator in self.default_generators:
if default_generator.name == generator_name:
return default_generator

return CMakeGenerator(generator_name)

# TODO: this method name is not great. Does anyone have a better idea for
# renaming it?
def get_best_generator(
Expand Down
47 changes: 42 additions & 5 deletions skbuild/setuptools_wrap.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import os.path
import sys
import argparse
import json

from contextlib import contextmanager
from distutils.errors import (DistutilsArgError,
Expand All @@ -23,6 +24,11 @@
except ImportError:
from io import StringIO

try:
from shutil import which
except ImportError:
from distutils.spawn import find_executable as which
Copy link
Contributor

@jcfr jcfr Mar 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, you could relocate the which function currently present in the test helpers.
See

def which(name, flags=os.X_OK):
"""Analogue of unix 'which'. Borrowed from the Twisted project, see
their licence here: https://twistedmatrix.com/trac/browser/trunk/LICENSE
Copied from ``pytest_shutil.cmdline.which`` to allow testing on
conda-forge where ``pytest-shutil`` is not available.
"""
result = []
exts = filter(None, os.environ.get('PATHEXT', '').split(os.pathsep))
path = os.environ.get('PATH', None)
if path is None:
return []
for p in os.environ.get('PATH', '').split(os.pathsep):
p = os.path.join(p, name)
if os.access(p, flags):
result.append(p)
for e in exts:
pext = p + e
if os.access(pext, flags):
result.append(pext)
return result


from setuptools import setup as upstream_setup
from setuptools.dist import Distribution as upstream_Distribution

Expand All @@ -31,7 +37,7 @@
install, install_lib, install_scripts,
bdist, bdist_wheel, egg_info,
sdist, generate_source_manifest, test)
from .constants import CMAKE_INSTALL_DIR
from .constants import CMAKE_INSTALL_DIR, CMAKE_ARGUMENTS_FILE
from .exceptions import SKBuildError, SKBuildGeneratorNotFoundError
from .utils import (mkdir_p, PythonModuleFinder, to_platform_path, to_unix_path)

Expand Down Expand Up @@ -290,6 +296,27 @@ def _should_run_cmake(commands, cmake_with_sdist):
return False


def _save_cmake_args(args):
"""Save the CMake arguments to disk"""
# We use JSON here because readability is more important than performance
try:
os.makedirs(os.path.dirname(CMAKE_ARGUMENTS_FILE))
except OSError:
pass

with open(CMAKE_ARGUMENTS_FILE, 'w+') as fp:
json.dump(args, fp)


def _load_cmake_args():
"""Load and return the CMake arguments from disk"""
try:
with open(CMAKE_ARGUMENTS_FILE) as fp:
return json.load(fp)
except (OSError, IOError, ValueError):
return None


# pylint:disable=too-many-locals, too-many-branches
def setup(*args, **kw): # noqa: C901
"""This function wraps setup() so that we can run cmake, make,
Expand Down Expand Up @@ -437,17 +464,27 @@ def setup(*args, **kw): # noqa: C901
# weight and when CMake is given multiple times a argument, only the last
# one is considered, let's prepend the one provided in the setup call.
cmake_args = skbuild_kw['cmake_args'] + cmake_args

# Used to confirm that the cmake executable is the same
cmake_cmd = [which('cmake')] + cmake_args
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think we should ask cmake what is its version.

After doing pip install -U cmake or updating the version on the system, the path would be the same


# Languages are used to determine a working generator
cmake_languages = skbuild_kw['cmake_languages']

try:
cmkr = cmaker.CMaker()
if not skip_cmake:
env = cmkr.configure(cmake_args,
cmake_source_dir=cmake_source_dir,
cmake_install_dir=skbuild_kw['cmake_install_dir'],
languages=cmake_languages)
# skip the configure step for a cached build
env = cmkr.get_cached_env()
if env is None or cmake_cmd != _load_cmake_args():
env = cmkr.configure(
cmake_args,
Copy link
Contributor

@jcfr jcfr Feb 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last thing would be to serialize the cmake_args into a file and compare it didn't change.

I suggest to add a function get_cached_cmake_args. Then configuration would be done only if the env is None or cmake_args != cached_cmake_args

cmake_source_dir=cmake_source_dir,
cmake_install_dir=skbuild_kw['cmake_install_dir'],
languages=cmake_languages
)
_save_cmake_args(cmake_cmd)

cmkr.make(make_args, env=env)
except SKBuildGeneratorNotFoundError as ex:
sys.exit(ex)
Expand Down
10 changes: 10 additions & 0 deletions tests/test_platform.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"""

import os
import sys
import platform
import pytest

Expand Down Expand Up @@ -109,3 +110,12 @@ def test_unsupported_platform(mocker):

assert failed
assert "Unsupported platform: bogus." in message


@pytest.mark.skipif(sys.platform != 'win32', reason='Requires Windows')
def test_cached_generator():
platform = get_platform()
generator = platform.get_generator('Ninja')
env = generator.env

assert 'Visual Studio' in env['LIB'] or 'Visual C++' in env['LIB']