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

avoid escaping spaces #265

Merged
merged 1 commit into from Nov 17, 2017

Conversation

3 participants
@neok-m4700
Contributor

neok-m4700 commented Nov 8, 2017

spaces in CYTHON_FLAGS are incorrectly escaped (at least on linux) when provided as command line arguments to the cython executable through CMake cache entries.

Here is a simple mwe (run $ bash build.sh in a terminal)
build.sh

#!/usr/bin/env bash
rm -r build; mkdir -p build; cd build

cmake \
-DCYTHON_FLAGS='-X boundscheck=False -X wraparound=False' \
-DCMAKE_MODULE_PATH="$SKBUILD_PREFIX/skbuild/resources/cmake" \
-DCMAKE_VERBOSE_MAKEFILE=ON ..

make || exit 1

python3 -c 'import fib; fib.fib(2000)'

CMakeLists.txt

cmake_minimum_required(VERSION 3.8)

project(fib)

find_package(Cython REQUIRED)
include(UseCython)

include_directories(${PYTHON_INCLUDE_DIRS})

add_cython_target(fib fib.pyx C PY3)
add_library(fib MODULE fib.c)
set_target_properties(fib PROPERTIES PREFIX "")

fib.pyx

def fib(n):
    """Print the Fibonacci series up to n."""
    a, b = 0, 1
    while b < n:
        print(b)
        a, b = b, a + b

without this patch, build.sh fails with the following output

<...>
cython --include-dir <...>/include/python3.6m -3 -X\ boundscheck=False\ -X\ wraparound=False <...>/fib.pyx --output-file <...>/build/fib.c
Error in compiler directive: boundscheck directive must be set to True or False, got 'False -X wraparound=False'
<...>
make[2]: *** [CMakeFiles/fib.dir/build.make:65: fib.c] Error 1
make[2]: Leaving directory <...>/build'
avoid escaping spaces
spaces in CYTHON_FLAGS are incorrectly escaped (at least on linux) when provided as command line arguments to the cython executable through CMake cache entries.

Here is a simple mwe (run `$ bash build.sh` in a terminal)
**build.sh**
```bash
#!/usr/bin/env bash
rm -r build; mkdir -p build; cd build

cmake \
-DCYTHON_FLAGS='-X boundscheck=False -X wraparound=False' \
-DCMAKE_MODULE_PATH="$SKBUILD_PREFIX/skbuild/resources/cmake" \
-DCMAKE_VERBOSE_MAKEFILE=ON ..

make || exit 1

python3 -c 'import fib; fib.fib(2000)'
```
**CMakeLists.txt**
```CMake
cmake_minimum_required(VERSION 3.8)

project(fib)

find_package(Cython REQUIRED)
include(UseCython)

include_directories(${PYTHON_INCLUDE_DIRS})

add_cython_target(fib fib.pyx C PY3)
add_library(fib MODULE fib.c)
set_target_properties(fib PROPERTIES PREFIX "")
```

**fib.pyx**
```python
def fib(n):
    """Print the Fibonacci series up to n."""
    a, b = 0, 1
    while b < n:
        print(b)
        a, b = b, a + b
```

without this patch, build.sh fails with the following output
```bash
<...>
cython --include-dir <...>/include/python3.6m -3 -X\ boundscheck=False\ -X\ wraparound=False <...>/fib.pyx --output-file <...>/build/fib.c
Error in compiler directive: boundscheck directive must be set to True or False, got 'False -X wraparound=False'
<...>
make[2]: *** [CMakeFiles/fib.dir/build.make:65: fib.c] Error 1
make[2]: Leaving directory <...>/build'
```
@thewtex

@neok-m4700 Thanks for the contribution! 👏

Well done on the patch and description of the issue 🥇

@thewtex

This comment has been minimized.

Member

thewtex commented Nov 17, 2017

CI errors are unrelated.

@thewtex thewtex merged commit b74f9b9 into scikit-build:master Nov 17, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@jcfr

This comment has been minimized.

Contributor

jcfr commented Nov 17, 2017

CI errors are unrelated.

These should now be fixed in #266

@jcfr jcfr referenced this pull request Nov 17, 2017

Open

test: add cython test case #267

@neok-m4700 neok-m4700 deleted the neok-m4700:patch-1 branch Nov 17, 2017

jcfr added a commit that referenced this pull request Nov 17, 2017

jcfr added a commit that referenced this pull request Nov 20, 2017

Merge pull request #268 from scikit-build/update-change-log
CHANGE: Add entry documenting fix for #265
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment