Repository navigation
Make add_openmp_flags_if_available() work for clang #382
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,24 @@ matrix: | |
| env: PYTHON_VERSION=3.6 PIP_DEPENDENCIES='git+https://github.com/sphinx-doc/sphinx.git#egg=sphinx coveralls' | ||
| CONDA_DEPENDENCIES="setuptools cython numpy pytest-cov" | ||
|
|
||
| # Test conda's clang | ||
| - os: osx | ||
| env: | ||
| - PYTHON_VERSION=3.5 | ||
| - CONDA_DEPENDENCIES="setuptools sphinx cython numpy pytest-cov clang llvm-openmp" | ||
| before_install: | ||
| - export TEST_OPENMP="True" | ||
| - export CC=clang | ||
|
|
||
| # Test gcc on OSX | ||
| - os: osx | ||
| env: | ||
| - PYTHON_VERSION=3.5 | ||
| - CONDA_DEPENDENCIES="setuptools sphinx cython numpy pytest-cov gcc" | ||
| before_install: | ||
| - export TEST_OPENMP="True" | ||
| - export CC=gcc | ||
|
|
||
| # Uncomment the following if there are issues in setuptools that we | ||
| # can't work around quickly - otherwise leave uncommented so that | ||
| # we notice when things go wrong. | ||
|
|
@@ -45,6 +63,16 @@ matrix: | |
| # CONDA_DEPENDENCIES='sphinx cython numpy pytest-cov' | ||
| # EVENT_TYPE='push pull_request cron' | ||
|
|
||
| before_install: | ||
|
|
||
| # Test OSX without OpenMP support | ||
| # Since the matrix OSX tests use the OS shipped version of clang, they double up | ||
| # as exploratory tests for when the shipped version has automatic OpenMP support. | ||
| # These tests will then fail and at such a time a new one should be added | ||
| # to explicitly remove OpenMP support. | ||
| - if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then export TEST_OPENMP="False"; | ||
| else export TEST_OPENMP="True"; fi | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll double check this, but if I remember correctly, it needs to be here so as to also work with the matrix permutations. There is no way to add this to them as they will be evaluated not at the correct stage, i.e. TRAVIS_OS_NAME is undefined. For the explicit tests it is, which is why I do so there also, but it isn't for the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regardless of OS, |
||
|
|
||
| install: | ||
|
|
||
| - git clone git://github.com/astropy/ci-helpers.git | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,13 @@ astropy-helpers Changelog | |
| by the ``sphinx-astropy`` package in conjunction with the ``astropy-theme-sphinx``, | ||
| ``sphinx-automodapi``, and ``numpydoc`` packages. [#368] | ||
|
|
||
| - openmp_helpers.py: Make add_openmp_flags_if_available() work for clang. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This changelog is way too verbose, any chance to cut it back a bit? E.g. no need for filenames, but can use double backticks to highlight the name of the new functions. |
||
| The necessary include, library, and runtime paths now get added to the C test code | ||
| used to determine if openmp works. | ||
| Autogenerator utility added ``openmp_enabled.is_openmp_enabled()`` | ||
| which can be called post build to determine state of OpenMP support. | ||
| [#382] | ||
|
|
||
|
|
||
| 3.0.2 (unreleased) | ||
| ------------------ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,12 +16,14 @@ | |
| import os | ||
| import sys | ||
| import glob | ||
| import time | ||
| import datetime | ||
| import tempfile | ||
| import subprocess | ||
|
|
||
| from distutils import log | ||
| from distutils.ccompiler import new_compiler | ||
| from distutils.sysconfig import customize_compiler | ||
| from distutils.sysconfig import customize_compiler, get_config_var | ||
| from distutils.errors import CompileError, LinkError | ||
|
|
||
| from .setup_helpers import get_compiler_option | ||
|
|
@@ -39,69 +41,223 @@ | |
| } | ||
| """ | ||
|
|
||
| def _get_flag_value_from_var(flag, var, delim=' '): | ||
| """ | ||
| Utility to extract ``flag`` value from `os.environ[``var``]` or, if not present, | ||
| from `distutils.sysconfig.get_config_var(``var``)`. | ||
| E.g. to get include path: _get_flag_value_from_var('-I', 'CFLAGS') | ||
| might return "/usr/local/include". | ||
|
|
||
| def add_openmp_flags_if_available(extension): | ||
| Notes | ||
| ----- | ||
| Not yet tested and therefore not yet supported on Windows | ||
| """ | ||
| Add OpenMP compilation flags, if available (if not a warning will be | ||
| printed to the console and no flags will be added) | ||
|
|
||
| Returns `True` if the flags were added, `False` otherwise. | ||
| if sys.platform.startswith('win'): | ||
| return None | ||
|
|
||
| # Simple input validation | ||
| if not var or not flag: | ||
| return None | ||
| flag_length = len(flag) | ||
| if not flag_length: | ||
| return None | ||
|
|
||
| # Look for var in os.eviron | ||
| try: | ||
| flags = os.environ[var] | ||
| except KeyError: | ||
| flags = None | ||
|
|
||
| # If os.environ was unsuccesful try in sysconfig | ||
| if not flags: | ||
| try: | ||
| flags = get_config_var(var) | ||
| except KeyError: | ||
| return None | ||
|
|
||
| # Extract flag from {var:value} | ||
| if flags: | ||
| for item in flags.split(delim): | ||
| if flag in item[:flag_length]: | ||
| return item[flag_length:] | ||
|
|
||
| return None | ||
|
|
||
| def _get_include_path(): | ||
| return _get_flag_value_from_var('-I', 'CFLAGS') | ||
|
|
||
| def _get_library_path(): | ||
| return _get_flag_value_from_var('-L', 'LDFLAGS') | ||
|
|
||
| def get_openmp_flags(): | ||
| """ | ||
| Utility for returning compiler and linker flags possibly needed for | ||
| OpenMP support. | ||
|
|
||
| ccompiler = new_compiler() | ||
| customize_compiler(ccompiler) | ||
| Returns | ||
| ------- | ||
| result : `{'compiler_flags':<flags>, 'linker_flags':<flags>}` | ||
|
|
||
| tmp_dir = tempfile.mkdtemp() | ||
| Notes | ||
| ----- | ||
| The flags returned are not tested for validity, use | ||
| `check_openmp_support(openmp_flags=get_openmp_flags())` to do so. | ||
| """ | ||
|
|
||
| start_dir = os.path.abspath('.') | ||
| compile_flags = [] | ||
| include_path = _get_include_path() | ||
| if include_path: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a comment explaining why you need to add library and include paths - e.g. does Python ignore environment variables by default? Sorry for being picky with comments, but we've been burned in the past with undocumented workarounds for issues in astropy-helpers, so I'm just making sure this is understandable by other developers :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think commenting in I believe the level of doc you want already exists here, where they are actually being used as flags passed to the compiler. # customize_compiler() extracts info from os.environ. If certain keys
# exist it uses these plus those from sysconfig.get_config_vars().
# If the key is missing in os.environ it is not extracted from
# sysconfig.get_config_var(). E.g. 'LDFLAGS' get left out, preventing
# clang from finding libomp.dylib because -L<path> is not passed to linker.
# Call get_openmp_flags() to get flags missed by customize_compiler().Is this cool as is? |
||
| compile_flags.append('-I' + include_path) | ||
|
|
||
| link_flags = [] | ||
| lib_path = _get_library_path() | ||
| if lib_path: | ||
| link_flags.append('-L' + lib_path) | ||
|
|
||
| if get_compiler_option() == 'msvc': | ||
| compile_flag = '-openmp' | ||
| link_flag = '' | ||
| compile_flags.append('-openmp') | ||
| else: | ||
| compile_flag = '-fopenmp' | ||
| link_flag = '-fopenmp' | ||
| compile_flags.append('-fopenmp') | ||
| link_flags.append('-fopenmp') | ||
| if lib_path: | ||
| link_flags.append('-Wl,-rpath,' + lib_path) | ||
|
|
||
| try: | ||
| return {'compiler_flags':compile_flags, 'linker_flags':link_flags} | ||
|
|
||
| def check_openmp_support(openmp_flags=None): | ||
| """ | ||
| Compile and run OpenMP test code to determine viable support. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| openmp_flags : dictionary, optional | ||
| Expecting `{'compiler_flags':<flags>, 'linker_flags':<flags>}`. | ||
| These are passed as `extra_postargs` to `compile()` and | ||
| `link_executable()` respectively. | ||
|
|
||
| Returns | ||
| ------- | ||
| result : bool | ||
| `True` if the test passed, `False` otherwise. | ||
| """ | ||
|
|
||
| ccompiler = new_compiler() | ||
| customize_compiler(ccompiler) | ||
|
|
||
| if not openmp_flags: | ||
| # customize_compiler() extracts info from os.environ. If certain keys | ||
| # exist it uses these plus those from sysconfig.get_config_vars(). | ||
| # If the key is missing in os.environ it is not extracted from | ||
| # sysconfig.get_config_var(). E.g. 'LDFLAGS' get left out, preventing | ||
| # clang from finding libomp.dylib because -L<path> is not passed to linker. | ||
| # Call get_openmp_flags() to get flags missed by customize_compiler(). | ||
| openmp_flags = get_openmp_flags() | ||
| compile_flags = openmp_flags['compiler_flags'] if 'compiler_flags' in openmp_flags else None | ||
| link_flags = openmp_flags['linker_flags'] if 'linker_flags' in openmp_flags else None | ||
|
|
||
| tmp_dir = tempfile.mkdtemp() | ||
| start_dir = os.path.abspath('.') | ||
|
|
||
| try: | ||
| os.chdir(tmp_dir) | ||
|
|
||
| # Write test program | ||
| with open('test_openmp.c', 'w') as f: | ||
| f.write(CCODE) | ||
|
|
||
| os.mkdir('objects') | ||
|
|
||
| # Compile, link, and run test program | ||
| ccompiler.compile(['test_openmp.c'], output_dir='objects', extra_postargs=[compile_flag]) | ||
| ccompiler.link_executable(glob.glob(os.path.join('objects', '*' + ccompiler.obj_extension)), 'test_openmp', extra_postargs=[link_flag]) | ||
| ccompiler.compile(['test_openmp.c'], output_dir='objects', extra_postargs=compile_flags) | ||
| ccompiler.link_executable(glob.glob(os.path.join('objects', '*' + ccompiler.obj_extension)), 'test_openmp', extra_postargs=link_flags) | ||
| output = subprocess.check_output('./test_openmp').decode(sys.stdout.encoding or 'utf-8').splitlines() | ||
|
|
||
| if 'nthreads=' in output[0]: | ||
| nthreads = int(output[0].strip().split('=')[1]) | ||
| if len(output) == nthreads: | ||
| using_openmp = True | ||
| is_openmp_supported = True | ||
| else: | ||
| log.warn("Unexpected number of lines from output of test OpenMP " | ||
| "program (output was {0})".format(output)) | ||
| using_openmp = False | ||
| is_openmp_supported = False | ||
| else: | ||
| log.warn("Unexpected output from test OpenMP " | ||
| "program (output was {0})".format(output)) | ||
| using_openmp = False | ||
|
|
||
| is_openmp_supported = False | ||
| except (CompileError, LinkError): | ||
|
|
||
| using_openmp = False | ||
| is_openmp_supported = False | ||
|
|
||
| finally: | ||
|
|
||
| os.chdir(start_dir) | ||
|
|
||
| return is_openmp_supported | ||
|
|
||
| def is_openmp_supported(): | ||
| """ | ||
| Utility to determine whether the build compiler | ||
| has OpenMP support. | ||
| """ | ||
|
|
||
| # don't log | ||
| log_threshold = log.set_threshold(log.FATAL) | ||
| ret = check_openmp_support() | ||
| log.set_threshold(log_threshold) | ||
| return ret | ||
|
|
||
| def add_openmp_flags_if_available(extension): | ||
| """ | ||
| Add OpenMP compilation flags, if supported (if not a warning will be | ||
| printed to the console and no flags will be added.) | ||
|
|
||
| Returns `True` if the flags were added, `False` otherwise. | ||
| """ | ||
|
|
||
| openmp_flags = get_openmp_flags() | ||
| using_openmp = check_openmp_support(openmp_flags=openmp_flags) | ||
|
|
||
| if using_openmp: | ||
| log.info("Compiling Cython extension with OpenMP support") | ||
| extension.extra_compile_args.append(compile_flag) | ||
| extension.extra_link_args.append(link_flag) | ||
| compile_flags = openmp_flags['compiler_flags'] if 'compiler_flags' in openmp_flags else None | ||
| link_flags = openmp_flags['linker_flags'] if 'linker_flags' in openmp_flags else None | ||
| log.info("Compiling Cython/C/C++ extension with OpenMP support") | ||
| extension.extra_compile_args.extend(compile_flags) | ||
| extension.extra_link_args.extend(link_flags) | ||
| else: | ||
| log.warn("Cannot compile Cython extension with OpenMP, reverting to non-parallel code") | ||
|
|
||
| return using_openmp | ||
|
|
||
| _IS_OPENMP_ENABLED_SRC = """ | ||
| # Autogenerated by {packagetitle}'s setup.py on {timestamp!s} | ||
|
|
||
| def is_openmp_enabled(): | ||
| \'\'\' | ||
| Autogenerated utility to determine, post build, whether the package | ||
| was built with or without OpenMP support. | ||
| \'\'\' | ||
| return {return_bool} | ||
| """[1:] | ||
|
|
||
| def generate_openmp_enabled_py(packagename, srcdir='.'): | ||
| """ | ||
| Utility for creating `openmp_enabled.is_openmp_enabled()` | ||
| used to determine, post build, whether the package was built | ||
| with or without OpenMP support. | ||
| """ | ||
|
|
||
| if packagename.lower() == 'astropy': | ||
| packagetitle = 'Astropy' | ||
| else: | ||
| packagetitle = packagename | ||
|
|
||
| epoch = int(os.environ.get('SOURCE_DATE_EPOCH', time.time())) | ||
| timestamp = datetime.datetime.utcfromtimestamp(epoch) | ||
|
|
||
| src = _IS_OPENMP_ENABLED_SRC.format(packagetitle=packagetitle, | ||
| timestamp=timestamp, | ||
| return_bool=is_openmp_supported()) | ||
|
|
||
| package_srcdir = os.path.join(srcdir, *packagename.split('.')) | ||
| is_openmp_enabled_py = os.path.join(package_srcdir, 'openmp_enabled.py') | ||
| with open(is_openmp_enabled_py, 'w') as f: | ||
| f.write(src) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure there is no need for these
before_installsections either, put those variable into theenvsection.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I added these here following the Travis documentation. Also, because of the generic
before_install, which then has to be overloaded per explicit job.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c.f. https://github.com/astropy/astropy-helpers/pull/382/files#r191006134