Skip to content

Re-render and disable shared Fortran libraries on OSX#69

Merged
jakirkham merged 2 commits intoconda-forge:masterfrom
qwhelan:osx
May 27, 2017
Merged

Re-render and disable shared Fortran libraries on OSX#69
jakirkham merged 2 commits intoconda-forge:masterfrom
qwhelan:osx

Conversation

@qwhelan
Copy link
Copy Markdown
Contributor

@qwhelan qwhelan commented May 16, 2017

While digging into why the Fortran tests were failing on OSX, I came across a log message saying the following:

checking if shared Fortran libraries are supported... no
configure: WARNING: Shared Fortran libraries not currently supported on Mac.
configure: WARNING: Allowing unsupported Fortran shared libraries due to use of --enable-unsupported flag

The --enable-unsupported flag is needed to build the high-level API and enable thread-safe (which is not supported by the library authors), so we can't turn that off. The solution here is to patch configure to disable the building of shared libraries for the Fortran API only.

Additionally, I have re-rendered the feedstock.

cc @jakirkham

@conda-forge-linter
Copy link
Copy Markdown

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/meta.yaml Outdated
@@ -21,7 +21,7 @@ source:

build:
number: 10
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please bump this to 11 as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@jakirkham
Copy link
Copy Markdown
Member

Thanks @qwhelan.

@jakirkham
Copy link
Copy Markdown
Member

jakirkham commented May 16, 2017

Also generally would note this mailing list thread where an HDF5 developer confirms shared libraries are unsupported/don't work properly on Mac with Fortran support enabled. This checks out with their CMake options for Mac as well (see the NO_MAC_FORTRAN option affects shared libraries). Not entirely sure how we got away with them this long. Though maybe @brtnfld (from the linked thread) can confirm and/or elaborate on whether this issue still exists and if there are workarounds.

@jakirkham
Copy link
Copy Markdown
Member

Would also appreciate if others took a look at this.

cc @astrofrog @stuarteberg

This was referenced May 16, 2017
@jakirkham
Copy link
Copy Markdown
Member

FTR I guess another potential fix to this problem would be to delete the dylib Fortran files produced by the HDF5 build on Mac. Though it would be a bit hacky, this would allow shared libraries to be kept for everything else that is not Fortran on Mac, which should minimize the change to the package. The files to delete would be libhdf5_fortran.dylib and libhdf5hl_fortran.dylib. We might also need to do something with the associated libtool files (probably either patching or removing them). It would be better to use some sort of configure option for this type of thing.

@stuarteberg
Copy link
Copy Markdown

I guess another potential fix to this problem would be to delete the dylib Fortran files produced by the HDF5 build on Mac

Yeah, that sounds more appealing, if it's not a huge pain. Most of my build scripts assume dynamic linking, and don't depend on the Fortran bindings.

@brtnfld
Copy link
Copy Markdown

brtnfld commented May 16, 2017 via email

@jakirkham
Copy link
Copy Markdown
Member

Do you know of a way to selectively disable shared libraries built, @brtnfld? Would be nice if we could keep all shared libraries except the Fortran ones if they are the issue.

@qwhelan
Copy link
Copy Markdown
Contributor Author

qwhelan commented May 17, 2017

One thing I noticed is that the make files will have to be patched to not execute the Fortran test suite after the dynamic Fortran libs are built.

Also, I do not have access to a Mac so some of these modifications may be difficult for me to experiment with.

@jakirkham
Copy link
Copy Markdown
Member

Sorry, I don't follow. FWICT the failing tests occur during make check, but these libraries are built in the previous make step. Why is it not possible to delete the dylibs between these two steps?

@qwhelan
Copy link
Copy Markdown
Contributor Author

qwhelan commented May 17, 2017

@jakirkham Whether Fortran libs should be built is passed to / stored by configure. The make check step assumes they exist as the relevant settings were set by configure, so some patching would be needed to disable the relevant tests.

@brtnfld
Copy link
Copy Markdown

brtnfld commented May 17, 2017 via email

] %}
{% for each_hdf5_lib in hdf5_libs %}
- test -f $PREFIX/lib/lib{{ each_hdf5_lib }}.a # [unix]
- test -f $PREFIX/lib/lib{{ each_hdf5_lib }}.dylib # [osx]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you please add this back?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jakirkham I can, but it will fail when executed unless we move the fortran lib into a different list. I'll revert this when I make the delete-between-make-and-make-check patch.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yep, we can break out the Fortran case no problem. Ok SGTM.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

recipe/meta.yaml Outdated
- test -f $PREFIX/lib/lib{{ each_hdf5_lib }}.dylib # [osx]
- test -f $PREFIX/lib/lib{{ each_hdf5_lib }}.so # [linux]
# Commented out as shared libs not currently supported for OSX
# - test -f $PREFIX/lib/lib{{ each_hdf5_lib }}.dylib # [osx]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

...and drop these lines.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reverted

recipe/meta.yaml Outdated
{% endfor %}
# {% for each_hdf5_lib in hdf5_libs %}
# - otool -L $PREFIX/lib/lib{{ each_hdf5_lib }}.dylib # [osx]
# {% endfor %}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also please uncomment this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

recipe/meta.yaml Outdated
- if not exist %PREFIX%\\Library\\bin\\{{ each_hdf5_lib }}.dll exit 1 # [win]
{% endfor %}

# Commented out as shared libs not currently supported for OSX
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

...and drop this line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@jakirkham
Copy link
Copy Markdown
Member

If configure detects enable-fortran and the OS is Darwin, then it will not build fortran shared libraries. This is what is built on my mac (no fortran shared libraries).

So I think @qwhelan has already explained this quite nicely above, but it seems --enable-unsupported is overriding this, which seems like a bad idea. In particular, the lines below in this build log tell us HDF5 is doing this.

checking if shared Fortran libraries are supported... no

configure: WARNING: Shared Fortran libraries not currently supported on Mac.

configure: WARNING: Allowing unsupported Fortran shared libraries due to use of --enable-unsupported flag

@brtnfld
Copy link
Copy Markdown

brtnfld commented May 17, 2017 via email

@jakirkham
Copy link
Copy Markdown
Member

Some contributors wanted to have --enable-threadsafe. However as we build the Fortran and C++ interfaces, that had to come with --enable-unsupported to satisfy existing use cases.

@brtnfld
Copy link
Copy Markdown

brtnfld commented May 17, 2017 via email

@stuarteberg
Copy link
Copy Markdown

If you need any help resolving this issue, @bluescarni might be able to help...

@qwhelan
Copy link
Copy Markdown
Contributor Author

qwhelan commented May 25, 2017

@jakirkham Ended up reading through the configure script and found a convenient place to patch H5_FORTRAN_SHARED="no" (there are no direct flags to set this given our other flags).

This builds the static version of the Fortran API on OSX (so h5fc is available) and the tests appear to be passing, along with being a fairly minimal changeset.

recipe/meta.yaml Outdated
number: 10
skip: True # [py==36]
number: 11
skip: True # [win and py==36]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor point, but should be able to change py==36 to py36.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will give that a shot - left it as is due to py36 not being listed on https://conda.io/docs/building/meta-yaml.html

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm...should be added. Raised issue ( conda/conda-docs#443 ) to get py36 added.

appveyor.yml Outdated

- TARGET_ARCH: x64
CONDA_PY: 36
CONDA_INSTALL_LOCN: C:\\Miniconda36-x64
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure why this is here as the skip should have ensure it wasn't added. Could you please try another re-render to see if that removes this? If not, we will need to investigate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, re-rendering removed this

@jakirkham
Copy link
Copy Markdown
Member

Ended up reading through the configure script and found a convenient place to patch H5_FORTRAN_SHARED="no" (there are no direct flags to set this given our other flags).

Nice find! Thanks for doing that @qwhelan. Maybe if we could set this with an environment variable that would solve this issue cleanly, @brtnfld.

Would point out to others that one can see shared Fortran libraries are disabled.

This builds the static version of the Fortran API on OSX (so h5fc is available) and the tests appear to be passing, along with being a fairly minimal changeset.

SGTM.

@qwhelan
Copy link
Copy Markdown
Contributor Author

qwhelan commented May 25, 2017

@jakirkham Incorporated all of your comments with this last push, should be good to merge once tests finish.

Copy link
Copy Markdown
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks again @qwhelan. 😄

@jakirkham jakirkham changed the title Re-render the feedstock and --disable-shared on OSX Re-render the feedstock and disable shared Fortran libraries on OSX May 26, 2017
@jakirkham jakirkham changed the title Re-render the feedstock and disable shared Fortran libraries on OSX Re-render and disable shared Fortran libraries on OSX May 26, 2017
@jakirkham
Copy link
Copy Markdown
Member

Might be worth updating the PR description since we ended up not using the flag, but using a patch to disable shared Fortran libraries.

@qwhelan
Copy link
Copy Markdown
Contributor Author

qwhelan commented May 26, 2017

@jakirkham Done.

@jakirkham jakirkham merged commit 9860a19 into conda-forge:master May 27, 2017
@jakirkham
Copy link
Copy Markdown
Member

It's in. Thanks @qwhelan.

@qwhelan qwhelan deleted the osx branch January 2, 2018 02:34
@hhslepicka hhslepicka mentioned this pull request Mar 18, 2021
1 task
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.

5 participants