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

MRG: move conditional compiling to C #528

Merged
merged 4 commits into from Dec 29, 2014

Conversation

Projects
None yet
3 participants
@matthew-brett
Member

matthew-brett commented Dec 29, 2014

We were using conditionals in the Cython file, but this means that we can't any
longer ship valid .c files with the release, and we would have to depend on
Cython at build time, even for releases.

Try moving the OpenMP defines into a C header so that the check for OpenMP
happens at C compile-time not Cython compile time.

matthew-brett added some commits Dec 29, 2014

NF: add safe openmp header / Cython pxd
Do check for openmp with C preprocessor, and then use C variable checks
to avoid using openmp constructs at build time.
RF: write config.h rather than config.pxi file
Signal that code should use C-level conditionals rather than
Cython-level conditionals by writing .h file instead of .pxi.
@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 29, 2014

Eleftherios - does this preserve the Open MP things you need? Is there a good way of checking? It looks to me as if it does.

matthew-brett added some commits Dec 29, 2014

BF: include_dirs for Cython pyx release compiles
Add the `src` include directory to allow all (bundlemin in fact) Cython
files to be compiled into the source release tarball.
BF: add more robust travis installs
Try a few times to install travis wheels before giving up.
@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 29, 2014

Yeah, it works. The tests look good too and the multithreading works in my machine. So, all good here.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 29, 2014

Matthew this says WIP here. Do you really need to add other commits or this is done? If this is done then it's already +1 from me.

@matthew-brett matthew-brett changed the title from WIP: trying to move conditional compiling to C to MRG: move conditional compiling to C Dec 29, 2014

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 29, 2014

Thx!

Garyfallidis added a commit that referenced this pull request Dec 29, 2014

Merge pull request #528 from matthew-brett/conditional-omp-fix
MRG: move conditional compiling to C

@Garyfallidis Garyfallidis merged commit 0eeef91 into nipy:master Dec 29, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@scoder

This comment has been minimized.

scoder commented on src/conditional_omp.h in 0ef1687 Jan 11, 2015

This should be a static variable. (same below)

This comment has been minimized.

Member

matthew-brett replied Jan 15, 2015

Sorry for my ignorance - you mean static int have_openmp = 1;? This is to make it less likely it can be accidentally modified? Or you meant const static int have_openmp = 1;?

This comment has been minimized.

scoder replied Jan 15, 2015

The first, but actually, why not just use a C #define ? It's not supposed to change anywhere else, right?

This comment has been minimized.

Member

matthew-brett replied Jan 15, 2015

Right - thanks - #556

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