cfitsio: add Darwin support #41254
Merged
cfitsio: add Darwin support #41254
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GrahamcOfBorg
added
6.topic: darwin
LnL7
suggested changes
May 30, 2018
| # Build 'Universal' binaries (i386 & x86_64 architectures) and | ||
| # use rpath token on Darwin 10.x or newer: | ||
| - SHLIB_LD="$CC -dynamiclib $C_UNIV_SWITCH -headerpad_max_install_names -install_name @rpath/lib\${PACKAGE}.\${CFITSIO_SONAME}\${SHLIB_SUFFIX} -compatibility_version \${CFITSIO_SONAME} -current_version \${CFITSIO_SONAME}.\${CFITSIO_MAJOR}.\${CFITSIO_MINOR}" | ||
| + SHLIB_LD="$CC -dynamiclib -headerpad_max_install_names -install_name @rpath/lib\${PACKAGE}.\${CFITSIO_SONAME}\${SHLIB_SUFFIX} -compatibility_version \${CFITSIO_SONAME} -current_version \${CFITSIO_SONAME}.\${CFITSIO_MAJOR}.\${CFITSIO_MINOR}" |
There was a problem hiding this comment.
We don't use rpaths on darwin, using @rpath/libcfitsio.5.dylib as the install_name will result in broken binaries/libraries since the stdenv expects us to be using absolute references to libraries instead.
If a package doesn't do this correctly we generally either patch the build or use install_name_tool to fixup the references at the end of the build. eg.
{
postPatch = ''
substituteInPlace cfitsio-universal/configure \
--replace "-install_name @rpath/lib" "-install_name $out/lib"
'';
}| # Shared-only build | ||
| buildFlags = "shared"; | ||
| patchPhase = '' sed -e '/^install:/s/libcfitsio.a //' -e 's@/bin/@@g' -i Makefile.in |
|
Thanks for the review @LnL7. I've fixed the |
LnL7
reviewed
May 31, 2018
| - SHLIB_LD="$CC -dynamiclib $C_UNIV_SWITCH -headerpad_max_install_names -install_name @rpath/lib\${PACKAGE}.\${CFITSIO_SONAME}\${SHLIB_SUFFIX} -compatibility_version \${CFITSIO_SONAME} -current_version \${CFITSIO_SONAME}.\${CFITSIO_MAJOR}.\${CFITSIO_MINOR}" | ||
| - ;; | ||
| - esac | ||
| + SHLIB_LD="$CC -dynamiclib -install_name ${out}/lib\${PACKAGE}.\${CFITSIO_SONAME}\${SHLIB_SUFFIX} -compatibility_version \${CFITSIO_SONAME} -current_version \${CFITSIO_SONAME}.\${CFITSIO_MAJOR}.\${CFITSIO_MINOR}" |
There was a problem hiding this comment.
${out}/lib/ is missing a trailing slash.
You can check the install name like this:
$ otool -D result/lib/libcfitsio.5.dylib
result/lib/libcfitsio.5.dylib:
/nix/store/iv4p3lnqck320m29c0nd4xz2abfc2f2p-cfitsio-3.430/libcfitsio.5.dylib
$ file /nix/store/iv4p3lnqck320m29c0nd4xz2abfc2f2p-cfitsio-3.430/libcfitsio.5.dylib
/nix/store/iv4p3lnqck320m29c0nd4xz2abfc2f2p-cfitsio-3.430/libcfitsio.5.dylib: cannot open `/nix/store/iv4p3lnqck320m29c0nd4xz2abfc2f2p-cfitsio-3.430/libcfitsio.5.dylib' (No such file or directory)
There was a problem hiding this comment.
You're right, thanks for noticing. Fixed with 73bc473.
LnL7
approved these changes
Jun 1, 2018
|
Thanks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
This PR adds support for the
cfitsiolibrary on Darwin. A couple of changes have been made in the configure script:1 - The detection of
/usr/bin/curl-configon Darwin has been turned off. As a result, the library does not support HTTPS, but I believe it is not supported on Linux either.2 - The flags to build a universal library have been removed because they cause a link error (missing
i386symbols).Things done
build-use-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)