[openssl] Handle INSTALL_NAME_DIR and OSX_DEPLOYMENT_TARGET for macOS shared builds #14785

LRFLEW · 2020-11-26T03:12:41Z

A few years ago, I made a PR here involving the dylib-id's of shared library builds for macOS and iOS (#4477). This updates the OpenSSL port to properly handle this functionality.

dylib-id's are weird, but put simply, they are strings stored inside of dylib files that are used when linking projects to it to say where the executable should look for the library at runtime. The default behavior generally is to set the dylib-id to the installation absolute path, as that's how it works for system-installed libraries. The default behavior for CMake and vcpkg (since my old PR), however, is to have the dylib-id be @rpath/<library>.dylib, which is generally what's used for user distributions (namely .app bundles). The old PR also included a way of specifying the dylib-id directory via a custom triplet.

Since the OpenSSL port (for "Unix") just wraps the automake system, this setting is not properly handled, and the dylib-id ends up being set to the absolute installation path. This PR uses install_name_tool to update the dylib-id's based on the CMake settings. (The other way of setting the dylib-id is to use the -install_name argument in the linker command, but doing this would require patching the autoconf setup, as the OpenSSL project doesn't include a way of changing the argument value.)

So I realized that another change I made in that old PR, the one for the minimum macOS target, also wasn't being handled by the OpenSSL port. The solution for this one was simpler, as it's the same as for OSX_SYSROOT. I decided to combine the changes into the same PR, as I see little reason to have them reviewed separately.

PhoebeHui

Thanks for the PR!

Could you update the Port-Version to 2 in CONTROL file?

LRFLEW · 2020-11-26T08:07:36Z

Could you update the Port-Version to 2 in CONTROL file?

I knew I forgot something. Fixed and Thanks

PhoebeHui · 2020-11-27T02:49:57Z

@LRFLEW, here is gdal failures with x64-linux and x64-osx from CI testing:

jpeg2000dataset.cpp: In member function ‘int JPEG2000Dataset::DecodeImage()’:
jpeg2000dataset.cpp:516:32: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
             if (poBand->iDepth != jas_image_cmptprec( psImage, iBand ) ||
                                ^
jpeg2000_vsil_io.cpp:154:1: error: invalid conversion from ‘int (*)(jas_stream_obj_t*, char*, unsigned int) {aka int (*)(void*, char*, unsigned int)}’ to ‘int (*)(jas_stream_obj_t*, const char*, unsigned int) {aka int (*)(void*, const char*, unsigned int)}’ [-fpermissive]
 };
 ^
make[2]: *** [../o/jpeg2000_vsil_io.lo] Error 1
make[1]: *** [jpeg2000-install-obj] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [frmts-target] Error 2

failure logs for x64-linux (7).zip

LRFLEW · 2020-11-27T08:42:56Z

@PhoebeHui I saw that error, but I can't figure out what to make of it. It's an error in an unrelated port in a file that should have no relation to any of the changes made here. Since this error appeared in the CI for an unrelated PR as well (#14799), I suspect this might be a problem from the master branch. I can try rebasing the PR against any changes made to master in case there's a fix there.

PhoebeHui · 2020-11-27T10:59:19Z

@LRFLEW, gdal depends on libpq, and libpq depends on openssl, so it might relate to this changes, I verified gdal:x64-linux on master, it installed successfully.

Starting package 9/9: gdal:x64-linux
Building package gdal[core]:x64-linux...
-- Downloading http://download.osgeo.org/gdal/3.1.3/gdal313.zip...
-- Extracting source /home/phoebe/vcpkg/downloads/gdal313.zip
-- Applying patch 0001-Fix-debug-crt-flags.patch
-- Applying patch 0002-Fix-build.patch
-- Applying patch 0003-Fix-static-build.patch
-- Applying patch 0004-Fix-std-fabs.patch
-- Applying patch 0005-Fix-cfitsio.patch
-- Using source at /home/phoebe/vcpkg/buildtrees/gdal/src/gdal313-b1dafdc415.cle                                                                                                 an
-- Configuring x64-linux-dbg
-- Configuring x64-linux-rel
-- Generating configure for x64-linux
-- Finished generating configure for x64-linux
-- Configuring x64-linux-dbg
-- Configuring x64-linux-rel
-- Building x64-linux-dbg
-- Installing x64-linux-dbg
-- Building x64-linux-rel
-- Installing x64-linux-rel
-- Installing: /home/phoebe/vcpkg/packages/gdal_x64-linux/share/gdal/usage
-- Installing: /home/phoebe/vcpkg/packages/gdal_x64-linux/share/gdal/copyright
-- Performing post-build validation
-- Performing post-build validation done
Building package gdal[core]:x64-linux... done
Installing package gdal[core]:x64-linux...

./vcpkg depend-info gdal
libjpeg-turbo:
liblzma:
sqlite3[tool]:
szip:
zlib:
curl[non-http, ssl, schannel, sspi, winssl]: zlib
hdf5[szip, zlib]: szip, zlib
libiconv:
openssl:
proj4[database]: sqlite3
tiff: libjpeg-turbo, liblzma, zlib
cfitsio: zlib
expat:
geos:
libgeotiff: libjpeg-turbo, proj4, tiff, zlib
libpng: zlib
libpq[openssl, zlib]: openssl, zlib
libwebp[nearlossless, unicode, simd]:
libxml2: libiconv, liblzma, zlib
netcdf-c: curl, hdf5
openjpeg:
gdal: cfitsio, curl, expat, geos, hdf5, libgeotiff, liblzma, libpng, libpq, libwebp, libxml2, netcdf-c, openjpeg, proj4, sqlite3, zlib

LRFLEW · 2020-11-28T03:29:56Z

@PhoebeHui I setup a Docker instance and tried building gdal on master, and I was able to confirm that it did succeed for me (once I worked around Docker killing the linker tasks). I then tried building my branch and could not reproduce the issue that the CI is seeing. I don't know what's different about my setup to the CI's exactly, but it's hard for me to debug it when I can't see the issue for myself.

Looking into the error message itself, it still seems to me like it's unrelated to this PR. The type error is happening when constructing an object of type jas_stream_ops_t, which appears to be defined in a header from another library called Jasper (#include <jasper/jasper.h> in jpeg2000_vsil_io.h). However, I'm not seeing Jasper in the output of the depend-info call you gave me. Where is it getting the header for it from? I suspect the problem is coming from having an inconsistent source for this header.

LRFLEW · 2020-11-28T04:10:20Z

Well, rebasing against master did seem to "fix" the error. This could just have been a matter of re-running the test. Perhaps the Jasper thing I mentioned should be looked into, but that would be a separate issue from this one.

PhoebeHui · 2020-11-30T09:36:27Z

@LRFLEW, thanks for looking into this issue!

It might due to Jasper, I will confirm this issue locally, anyway, it should be another issue, should not block this PR.
Checked the CI test results, gdal:x64-linux and x64-osx have been tested and passed.

PhoebeHui · 2020-12-01T06:17:06Z

Confirmed this issue on master, and found when jasper installed before gdal, gdal will fail on linux and osx. I will look into the failures. It is not relate to this changes. please ignore it.

jpeg2000dataset.cpp: In member function ‘int JPEG2000Dataset::DecodeImage()’:
jpeg2000dataset.cpp:516:32: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
             if (poBand->iDepth != jas_image_cmptprec( psImage, iBand ) ||
                                ^
jpeg2000_vsil_io.cpp:154:1: error: invalid conversion from ‘int (*)(jas_stream_obj_t*, char*, unsigned int) {aka int (*)(void*, char*, unsigned int)}’ to ‘int (*)(jas_stream_obj_t*, const char*, unsigned int) {aka int (*)(void*, const char*, unsigned int)}’ [-fpermissive]
 };
 ^
make[2]: *** [../o/jpeg2000_vsil_io.lo] Error 1
make[1]: *** [jpeg2000-install-obj] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [frmts-target] Error 2

PhoebeHui · 2020-12-01T06:18:12Z

@strega-nil, could you help further review?

strega-nil · 2020-12-01T22:30:30Z

LGTM! Thanks @LRFLEW

Extending microsoft#39313 to fix issues such as microsoft#14785 with openssl where libssl wasn't pointing to the rpath fixed id of libcrypto.

#8) Extending microsoft#39313 to fix issues such as microsoft#14785 with openssl where libssl wasn't pointing to the rpath fixed id of libcrypto. Co-authored-by: Derek Cyrus-Chow <derek.chow@catapult.com>

LRFLEW mentioned this pull request Nov 26, 2020

vcpkg dependency builds for macOS OpenRCT2/Dependencies#25

Merged

5 tasks

LRFLEW changed the title ~~[openssl] Handle INSTALL_NAME_DIR for macOS shared builds~~ [openssl] Handle INSTALL_NAME_DIR and OSX_DEPLOYMENT_TARGET for macOS shared builds Nov 26, 2020

NancyLi1013 assigned PhoebeHui Nov 26, 2020

PhoebeHui requested changes Nov 26, 2020

View reviewed changes

PhoebeHui added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist requires:author-response labels Nov 26, 2020

LRFLEW force-pushed the openssl-install-name branch from eb029ca to 0ee0ebc Compare November 26, 2020 07:57

LRFLEW requested a review from PhoebeHui November 26, 2020 08:07

LRFLEW added 2 commits November 27, 2020 21:31

[openssl] Handle INSTALL_NAME_DIR for macOS shared builds

6fb1cce

[openssl] Handle OSX_DEPLOYMENT_TARGET for macOS shared builds

620bbd9

LRFLEW force-pushed the openssl-install-name branch from 0ee0ebc to 620bbd9 Compare November 28, 2020 03:31

PhoebeHui removed the requires:author-response label Dec 1, 2020

PhoebeHui approved these changes Dec 1, 2020

View reviewed changes

PhoebeHui mentioned this pull request Dec 1, 2020

[vcpkg baseline] [gdal] Fix build errors on linux and osx #14866

Merged

strega-nil merged commit 0a45709 into microsoft:master Dec 1, 2020

PhoebeHui added info:reviewed Pull Request changes follow basic guidelines and removed requires:discussion labels Dec 2, 2020

This was referenced Jul 12, 2024

Fix osx-dynamic install names for updated dependent shared library ids #39889

Open

[openssl] on *-osx-dynamic triplets don't have install_name set properly #30805

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[openssl] Handle INSTALL_NAME_DIR and OSX_DEPLOYMENT_TARGET for macOS shared builds #14785

[openssl] Handle INSTALL_NAME_DIR and OSX_DEPLOYMENT_TARGET for macOS shared builds #14785

LRFLEW commented Nov 26, 2020 •

edited

Loading

PhoebeHui left a comment

LRFLEW commented Nov 26, 2020

PhoebeHui commented Nov 27, 2020

LRFLEW commented Nov 27, 2020

PhoebeHui commented Nov 27, 2020

LRFLEW commented Nov 28, 2020

LRFLEW commented Nov 28, 2020

PhoebeHui commented Nov 30, 2020 •

edited

Loading

PhoebeHui commented Dec 1, 2020

PhoebeHui commented Dec 1, 2020

strega-nil commented Dec 1, 2020

[openssl] Handle INSTALL_NAME_DIR and OSX_DEPLOYMENT_TARGET for macOS shared builds #14785

[openssl] Handle INSTALL_NAME_DIR and OSX_DEPLOYMENT_TARGET for macOS shared builds #14785

Conversation

LRFLEW commented Nov 26, 2020 • edited Loading

PhoebeHui left a comment

Choose a reason for hiding this comment

LRFLEW commented Nov 26, 2020

PhoebeHui commented Nov 27, 2020

LRFLEW commented Nov 27, 2020

PhoebeHui commented Nov 27, 2020

LRFLEW commented Nov 28, 2020

LRFLEW commented Nov 28, 2020

PhoebeHui commented Nov 30, 2020 • edited Loading

PhoebeHui commented Dec 1, 2020

PhoebeHui commented Dec 1, 2020

strega-nil commented Dec 1, 2020

LRFLEW commented Nov 26, 2020 •

edited

Loading

PhoebeHui commented Nov 30, 2020 •

edited

Loading