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 --enable-ros3-vfd build flag #124

Merged
merged 7 commits into from
Aug 7, 2020

Conversation

djhoese
Copy link
Contributor

@djhoese djhoese commented Aug 4, 2020

The whole point of #122 was to play with the new read-only S3 VFD functionality of HDF5 1.12. Apparently I forgot to check if it actually got enabled by default (it doesn't). This PR enables the feature and as far as I know does not have any additional requirements or effect any other features of the library since it should be a libcurl-based contact to a remote S3 storage. More info here:

https://portal.hdfgroup.org/display/HDF5/Configuration+and+Setup+for+HDF5+Read+Only+S3+VFD

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-linter
Copy link

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 (recipe) and found it was in an excellent condition.

recipe/build.sh Outdated
@@ -37,6 +37,7 @@ fi
--enable-unsupported \
--enable-using-memchecker \
--enable-static=yes \
--enable-ros3-vfd \
Copy link
Member

Choose a reason for hiding this comment

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

last element shouldn't have a trailing slash

Copy link
Contributor Author

@djhoese djhoese Aug 4, 2020

Choose a reason for hiding this comment

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

I thought it was odd but the static one had it too. Any idea why that works?

Edit: I mean master had it that way so I copied it.

Copy link
Member

Choose a reason for hiding this comment

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

Probably because there are just blank lines following it

@djhoese
Copy link
Contributor Author

djhoese commented Aug 4, 2020

checking if the Read-Only S3 virtual file driver (VFD) is enabled... no
configure: error: The Read-Only S3 VFD was requested but cannot be built.
                      Please check that openssl and cURL are available on your
                      system, and/or re-configure without option
                      --enable-ros3-vfd.

Looks like SSL and curl are needed. For some reason I assumed they would have already been added to the conda-forge build. I'm guessing that makes this a bigger deal to include (people maybe not wanting ssl and curl installed)?

@scopatz
Copy link
Member

scopatz commented Aug 5, 2020

I mean, I don't think it is that big of a deal to add ssl and curl deps

@djhoese
Copy link
Contributor Author

djhoese commented Aug 7, 2020

At a loss again. Configure seems to find the headers for curl just fine:

2020-08-06T20:56:17.8136018Z checking curl/curl.h usability... yes
2020-08-06T20:56:17.8485835Z checking curl/curl.h presence... yes
2020-08-06T20:56:17.8490187Z checking for curl/curl.h... yes

But then later during compilation:

2020-08-06T21:05:52.0771711Z Making install in src
2020-08-06T21:05:52.4242117Z make[1]: Entering directory '/home/conda/feedstock_root/build_artifacts/hdf5_split_1596747167656/work/src'
2020-08-06T21:05:52.4243388Z   CC       H5FDros3.lo
2020-08-06T21:05:52.9916273Z In file included from H5FDros3.c:35:0:
2020-08-06T21:05:52.9917163Z H5FDs3comms.h:60:10: fatal error: curl/curl.h: No such file or directory
2020-08-06T21:05:52.9917621Z  #include <curl/curl.h>
2020-08-06T21:05:52.9917909Z           ^~~~~~~~~~~~~
2020-08-06T21:05:52.9918201Z compilation terminated.

@scopatz
Copy link
Member

scopatz commented Aug 7, 2020

Maybe you need to pass the location of curl into the configure script? Sometimes that is needed

@djhoese
Copy link
Contributor Author

djhoese commented Aug 7, 2020

I'm 90% the configure script is the thing that is giving the first output of "checking" and successfully finding curl.h. Otherwise would I do --with-curl="${PREFIX}/include/curl?

@djhoese
Copy link
Contributor Author

djhoese commented Aug 7, 2020

@isuruf I suppose we should just remove the verbosity stuff and then this is good for review?

@isuruf
Copy link
Member

isuruf commented Aug 7, 2020

Why remove verbose stuff? It's good for debugging when something fails.

@djhoese
Copy link
Contributor Author

djhoese commented Aug 7, 2020

Ah ok. I just thought you were adding it to help figure out why curl wasn't showing up. Nevermind then. Ready for review.

@isuruf
Copy link
Member

isuruf commented Aug 7, 2020

I just thought you were adding it to help figure out why curl wasn't showing up.

I was. It'll still be useful for future when something goes wrong.

@scopatz scopatz merged commit cc57090 into conda-forge:master Aug 7, 2020
@scopatz
Copy link
Member

scopatz commented Aug 7, 2020

Great work @djhoese and thanks @isuruf!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants