Skip to content
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 HDF5 (pt. 2) - merge when passing #199

Merged
merged 11 commits into from
Mar 25, 2016
Merged

Conversation

jakirkham
Copy link
Member

This continues off @groutr's excellent work in this PR ( #192 ) to get a build of HDF5 going.

Initially thought Travis CI was failing builds due to a lack of output from the tests. However, it has been determine that one of the test portions was causing a hang. Namely the cache tests. Here we simply patch the test/Makefile.in to skip the cache test program. This appears to be enough to get Travis CI to run through. We go ahead and apply this patch everywhere even though Travis CI was the only one that needed. My best guess is this test used too much memory, which led to hang.

@jakirkham
Copy link
Member Author

cc @ocefpaf

Maybe it won't be that hacky at all. It turns out there is a --disable-silent-rules argument. I just passed this to ./configure. Hopefully, that won't be too noisy though or we will have different problems with Travis and or the other ones.

./configure --prefix=$PREFIX \
--enable-linux-lfs --with-zlib \
--with-pthread=yes --enable-cxx --with-default-plugindir=$PREFIX/lib/hdf5/plugin \
--disable-silent-rules # To make Travis happy with the level of activity.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the argument to add a bit more noise to the Travis CI build. Hopefully, that is the right amount without going overboard the other way.

@jakirkham
Copy link
Member Author

Appears this is not an issue with the amount of noise, but actually may be a memory issue with the cache test. Attempting to patch this out of the test running step so that maybe we can complete the test suite.

@jakirkham jakirkham force-pushed the add_hdf5 branch 2 times, most recently from b7993cf to 5e097ff Compare March 25, 2016 03:10
This test appears to be pretty resource intensive and is grinding Travis
CI to a halt. As a result, we will simply disable running this test and
see if that is sufficient in order to run the rest of the test suite.
@@ -2,7 +2,8 @@

./configure --prefix=$PREFIX \
--enable-linux-lfs --with-zlib \
--with-pthread=yes --enable-cxx --with-default-plugindir=$PREFIX/lib/hdf5/plugin
--with-pthread=yes --enable-cxx --with-default-plugindir=$PREFIX/lib/hdf5/plugin \
--disable-silent-rules # To make Travis happy with the level of activity.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This attempts to increase the noise level.

@jakirkham
Copy link
Member Author

So, I have added a patch here, which simply does not run the cache program from make check. The alternative would be not to run make check, which was deemed unacceptable. This at least allows the rest of the tests (other than cache) to run. None of those have been seen to have problems on any platform.

This appears to allow for the Mac Travis CI build to complete. Without it, the build hangs and is terminated after no response for ~10mins.

It is unlikely this patch would be accepted upstream as it skips one of their tests. So, I don't plan to submit it. However, a discussion could begin about making this test less resource intensive (assumed to be memory intensive). Depending on the nature of the test, this may or may not be feasible.

@jakirkham
Copy link
Member Author

🎉 Travis CI passes! 🎉

url: http://www.hdfgroup.org/ftp/HDF5/current/src/hdf5-{{version}}.tar.gz
md5: b8ed9a36ae142317f88b0c7ef4b9c618
patches:
- test_Makefile.in.patch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd want a really good comment in here to explain the rationale. Maybe provide a link back to the original PR too.

P.S. I'm happy for this to be done in the feedstock once merged.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem. Are you ok with the rationale?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Slightly concerned - maybe we should have an environment variable which allows us to turn stuff on if we are conda-forge building stuff. Like:

patches:
    - test_Makefile.in.patch  # [os.environ.get(conda-forge)]

@pelson pelson merged commit f125dbf into conda-forge:master Mar 25, 2016
@jakirkham jakirkham deleted the add_hdf5 branch March 25, 2016 04:50
@jakirkham jakirkham mentioned this pull request Mar 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants