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 Fortran bindings #28
Conversation
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
@@ -32,22 +32,25 @@ requirements: | |||
- python # [win] | |||
- cmake >=3.1 | |||
- zlib 1.2* | |||
- gcc # [not win] |
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 thought we only needed gcc
for Linux and not OSX. Is this right @jakirkham ?
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.
Not when it comes to Fortran. That is the only way we have to build Fortran on OS X. We will need to make sure the clang
stuff stays entact. We don't want this to start shipping libgcc
on OS X if we can avoid it.
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.
Yes agree about libfortran
but gcc
should have the linux
selector instead? Is that right?
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.
Oh you mean the line below.
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.
Sorry I misread. Though we do have to do something about that line too.
Unfortunately we need gcc
on OS X too if we want to have access to gfortran
. Mac doesn't provide a gfortran
compiler. I believe they use to, but that was on 10.4. Fortunately, the work we did getting clang
to be the compiler and libc++
used should actually help us here. As a result, we should only need to ship libgfortran
with this on OS X. I just went through a similar ordeal with scipy
. So, am pretty confident that this should work. We still want to review this carefully nonetheless.
Yeah, actually, I'm going to propose the exact same thing we did with |
@@ -34,22 +34,24 @@ requirements: | |||
- libtool # [unix] | |||
- cmake >=3.1 | |||
- zlib 1.2* | |||
- gcc # [not win] |
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.
Change # [not win]
-> # [osx]
.
These changes were needed for SciPy to ensure that we did not need to ship |
@@ -11,13 +11,17 @@ source activate "${CONDA_DEFAULT_ENV}" | |||
if [ "$(uname)" == "Darwin" ] | |||
then | |||
export CXX="${CXX} -stdlib=libc++" | |||
export DYLD_FALLBACK_LIBRARY_PATH=$PREFIX/lib | |||
export LIBRARY_PATH="${PREFIX}/lib" |
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.
Oh this last line should be for both platforms. In particular, we need this for Linux.
Also forgot to mention please do this locally in the top level of this repo.
This should let us use the |
@jakirkham - thanks for the tips! Strangely, re-rendering led to the Python 3 builds being dropped from AppVeyor, so I did not include that change in the latest commit. Should I have done? |
@@ -26,7 +26,7 @@ install: | |||
|
|||
conda config --set show_channel_urls true | |||
conda update --yes conda | |||
conda install --yes conda-build jinja2 anaconda-client | |||
conda install --yes conda-build=1.20.0 jinja2 anaconda-client |
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 know it generated this, but we should not include it. See this issue ( #23 ) for background as to why we removed it.
Of course.
Nope, you were right not to. I think you are running into this issue ( conda-tools/conda-build-all#49 ). If you are able to create a reproducer and add it to that issue, that would be amazing. I've had such trouble trying to get anything that would be reproducible. |
Looks like everything is passing! :) |
Thanks @astrofrog. Could I ask that we add some test code here? I would like to see a couple test programs (it could be copied from HDF5's examples for all I care). Something for Fortran, C, and C++. I expect this will work, but I want to be very sure that it does. These tests can be Unix only. Building in the test phase for Windows has proved very problematic and we have not changed that build AFAICT. |
@jakirkham - no problem, I'll add some test code today! |
I've added some test cases that I copied from the HDF5 examples - I picked one that includes compression etc. to really test things. I checked and included the HDF5 license to make sure this is acceptable. |
002d312
to
ec9317e
Compare
- python 2.7.* # [win and py27] | ||
- python 3.4.* # [win and py34] | ||
- python 3.5.* # [win and py35] | ||
- python {{ environ['PY_VER'] + '*' }} # [win] |
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.
Need to add gcc
for OS X only.
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 thought this was already the case:
- gcc # [osx]
I think GitHub messed up the lines for the comments, @jakirkham can you clarify which line you were referring to?
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.
Ah, but I'm suggesting we make it a test requirement. Travis CI is complaining we don't have gfortran
. So this should fix it.
Once we get this one worked out, I'd like to test it out some locally. I really want to be sure we don't accidentally break |
@jakirkham - absolutely! I wonder if it would be worth pushing my branch to a prerelease branch in the feedstock so that we can all test out the builds by going to a particular label? |
Ok, so the MacOS X build is now failing with:
I'm assuming this should not be fixed with the fallback path, because we can't expect users to actually do this too. Does anyone know why the libgfortran library wouldn't get picked up at this point? (I'm pretty sure it's installed since it's in the runtime dependencies). |
(just rebased) |
Trying to use this with |
I did some tests with HDF5 from the test label on MacOS X. The environment I am using has:
The h5py tests pass:
and likewise the pytables tests pass:
On Linux, the h5py tests pass:
and I'm currently running the 'heavy' pytables test suite. |
The full test suite for pytables ran (in 3 hours!) with no issues:
These packages didn't need re-building or anything, I just used the latest conda-forge versions. @jakirkham @pelson - any other tests you would like me to do before we can merge this? |
From my perspective, I see no blockers, but I haven't been that close to the discussion. I think @astrofrog's testing looks pretty extensive - @jakirkham, do you have any remaining concerns that @astrofrog can address, or are you OK to let this fly? |
So was PyTables tested on Linux too, @astrofrog? Am a little unclear. Not worried about the heavy test suite on both, but just confirming that the standard test suite works on both OS X and Linux. |
@jakirkham - yes, I ran the normal pytables suite on Mac and the heavy one on Linux, and the h5py test suite on both Mac and Linux. |
@jakirkham - should I increase the build number btw? or does that happen automatically? |
Yep, please bump the build number and then will merge. |
@jakirkham - I think this is ready to go - thanks! |
Thanks @astrofrog. |
They changed the website in the last couple hours so the source download is failing. 😒 When you have a chance, could you please add a PR to correct this, @astrofrog? |
@jakirkham - done in #42 |
Thanks for doing that, @astrofrog. |
Closes #11
Continued version of #14