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
base: master
from

Conversation

2 participants
@ghost

ghost commented Feb 24, 2018

Closes gh-289.

xoviat added some commits Feb 24, 2018

xoviat
xoviat
xoviat
xoviat
@codecov-io

This comment has been minimized.

codecov-io commented Feb 24, 2018

Codecov Report

Merging #301 into master will decrease coverage by 0.3%.
The diff coverage is 89.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #301      +/-   ##
==========================================
- Coverage   90.83%   90.52%   -0.31%     
==========================================
  Files          26       27       +1     
  Lines        1036     1098      +62     
  Branches      184      196      +12     
==========================================
+ Hits          941      994      +53     
- Misses         65       69       +4     
- Partials       30       35       +5
Impacted Files Coverage Δ
skbuild/constants.py 100% <100%> (ø) ⬆️
skbuild/platform_specifics/abstract.py 97.64% <60%> (-2.36%) ⬇️
skbuild/compat.py 87.5% <87.5%> (ø)
skbuild/setuptools_wrap.py 95.67% <91.66%> (-0.33%) ⬇️
skbuild/cmaker.py 74.16% <94.73%> (+0.86%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5fa09f...5bb1d1f. Read the comment docs.

xoviat
@jcfr

This comment has been minimized.

Contributor

jcfr commented Feb 25, 2018

Very nice 👍

env = cmkr.get_cached_env()
if env is None:
env = cmkr.configure(
cmake_args,

This comment has been minimized.

@jcfr

jcfr Feb 25, 2018

Contributor

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

xoviat added some commits Feb 25, 2018

xoviat
xoviat
xoviat
@ghost

This comment has been minimized.

ghost commented Mar 4, 2018

@jcfr Can we put this in?

@@ -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.txt")

This comment has been minimized.

@jcfr

jcfr Mar 4, 2018

Contributor

CMakeArguments.json ?

languages=cmake_languages)
# skip the configure step for a cached build
env = cmkr.get_cached_env()
if env is None or cmake_args != _load_cmake_args():

This comment has been minimized.

@jcfr

jcfr Mar 4, 2018

Contributor

I suggest we handle the case when a different cmake is being used.
Few approaches:

  • (1) abort and mention that a different version of cmake is being used
  • (2) or mention that a different version is detected and re-configure

This comment has been minimized.

@ghost

ghost Mar 4, 2018

The current approach is to re-configure, so I'll do that.

xoviat added some commits Mar 4, 2018

xoviat
xoviat
xoviat
try:
from shutil import which
except ImportError:
from distutils.spawn import find_executable as which

This comment has been minimized.

@jcfr

jcfr Mar 4, 2018

Contributor

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

@@ -459,6 +464,9 @@ 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

This comment has been minimized.

@jcfr

jcfr Mar 4, 2018

Contributor

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

@ghost

This comment has been minimized.

ghost commented Mar 4, 2018

@jcfr Wouldn't the version string be enough, even if CMake located differently on the path?

@ghost

This comment has been minimized.

ghost commented Mar 4, 2018

No, it wouldn't.

@jcfr

This comment has been minimized.

Contributor

jcfr commented Mar 4, 2018

Wouldn't the version string be enough, even if CMake located differently on the path?

I think the version string alone would be sufficient. I don't think we should check for the path.

Here code getting the version that could be refactored.

try:
subprocess.check_call(['cmake', '--version'])
except (OSError, subprocess.CalledProcessError):
raise SKBuildError(
"Problem with the CMake installation, aborting build.")

@ghost

This comment has been minimized.

ghost commented Mar 4, 2018

I checked the build.ninja, and it has the full path to the CMake executable.

@jcfr

This comment has been minimized.

Contributor

jcfr commented Mar 4, 2018

Good point. Also any add_custom_command using CMAKE_COMMAND would have the full path too, it means we need to use which as you started to do, and also check for the version

xoviat added some commits Mar 4, 2018

xoviat
xoviat
except (OSError, subprocess.CalledProcessError):
raise SKBuildError(
"Problem with the CMake installation, aborting build.")
if sys.version_info > (3, 0):
version_string = version_string.decode()

This comment has been minimized.

@ghost

ghost Mar 4, 2018

I have no idea what happens on PY2. @jcfr Ideas?

xoviat added some commits Mar 4, 2018

xoviat
xoviat
@ghost

This comment has been minimized.

ghost commented Mar 4, 2018

@jcfr I think everything is fine now, but for the one line above (which may be, but I cannot confirm, is okay).

@jcfr jcfr referenced this pull request Mar 8, 2018

Closed

Where is "xoviat" ? #309

@jcfr

This comment has been minimized.

Contributor

jcfr commented May 9, 2018

Closing. Superseded by #318

@jcfr jcfr closed this May 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment