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

ZLIB / itkzlib-ng in ITK v5.3 doesn't use @rpath on macOS #4084

Closed
kislinsk opened this issue Jun 20, 2023 · 11 comments
Closed

ZLIB / itkzlib-ng in ITK v5.3 doesn't use @rpath on macOS #4084

kislinsk opened this issue Jun 20, 2023 · 11 comments
Labels
type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Compiler Compiler support or related warnings

Comments

@kislinsk
Copy link
Contributor

kislinsk commented Jun 20, 2023

Description

After upgrading to ITK v5.3 all of our runtimes fail on macOS with messages like:

dyld: Library not loaded: lib/libitkzlib-5.3.1.dylib
  Referenced from: MITK-build/lib/libMitkCore.dylib
  Reason: image not found

Calling otool -L on libMitkCore.dylib revealed that - while all other ITK libraries are using @rpath to be located - itkzlib is an exception, for example:

...
@rpath/libitksys-5.3.1.dylib <- as expected
lib/libitkzlib-5.3.1.dylib <- yikes!
...

As it turns out, zlib-ng deliberately turns off @rpath here:

https://github.com/InsightSoftwareConsortium/ITK/blob/v5.3.0/Modules/ThirdParty/ZLIB/src/itkzlib-ng/CMakeLists.txt#L1107-L1108

Since it is a relative path, it can be classified as a bug, as it cannot be the intention to make such a location depending not even on the executable/loader path but the working directory (!) at runtime and after upgrading this dependency in ITK with the UpdateFromUpstream.sh script, they have at least fixed it to an absolute path, which kind of fixes the issue for us but still is a little unpleasant, since all the other ITK libraries are using @rpath.

I don't know why this wasn't reported earlier since it broke ITK on macOS except you copy the itkzlib library into a lib subdirectory of the working directory as a workaround.

Expected behavior

Binaries linking to ITK on macOS should just work without copying around libraries.

Actual behavior

libitkzlib cannot be located/loaded without extra measures.

Reproducibility

Consistently fails for us on macOS Bug Sur, Catalina, and Ventura.

Versions

v5.3.0

Environment

macOS

Additional Information

While upgrading the zlib-ng dependency works kind of fine with an absolute path, removing the line linked above would lead to the fully expected behavior of using @rpath just like all other ITK libraries. However, I do not know if you are willing to patch the dependency as I doubt that the upstream would switch to the desired and default @rpath method.

BTW, CPack is able to resolve the location of itkzlib for installers so this is primarily an issue in developer environments.

@kislinsk kislinsk added the type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances label Jun 20, 2023
@thewtex
Copy link
Member

thewtex commented Jun 23, 2023

Upstream patch in progress here: zlib-ng/zlib-ng#1524

@thewtex thewtex added the type:Compiler Compiler support or related warnings label Jun 23, 2023
@dzenanz
Copy link
Member

dzenanz commented Jun 29, 2023

@kislinsk does #4093 fix this?

@kislinsk
Copy link
Contributor Author

@kislinsk does #4093 fix this?

Unfortunately it introduces CMake errors:

-- ZLIB_HEADER_VERSION: 1.2.13
-- ZLIBNG_HEADER_VERSION: 2.1.3
-- Arch detected: 'x86_64'
-- Basearch of 'x86_64' has been detected as: 'x86'
-- Architecture-specific source files: arch/x86/x86_features.c;arch/x86/slide_hash_avx2.c;arch/x86/chunkset_avx2.c;arch/x86/compare256_avx2.c;arch/x86/adler32_avx2.c;arch/x86/adler32_avx512.c;arch/x86/adler32_avx512_vnni.c;arch/x86/adler32_sse42.c;arch/x86/insert_string_sse42.c;arch/x86/chunkset_sse2.c;arch/x86/compare256_sse2.c;arch/x86/slide_hash_sse2.c;arch/x86/adler32_ssse3.c;arch/x86/chunkset_ssse3.c;arch/x86/crc32_pclmulqdq.c;arch/x86/crc32_vpclmulqdq.c
CMake Error at CMakeLists.txt:919 (file):
  file STRINGS file
  "/Users/kislinsk/MITK-superbuild/ep/src/ITK/zconf-ng.h.in" cannot be read.
Call Stack (most recent call first):
  CMakeLists.txt:930 (generate_cmakein)


CMake Error: File /Users/kislinsk/MITK-superbuild/ep/src/ITK-build/zconf-ng.h.cmakein does not exist.
CMake Error at CMakeLists.txt:1179 (configure_file):
  configure_file Problem configuring file


CMake Error: File /Users/kislinsk/MITK-superbuild/ep/src/ITK/zlib-ng.h.in does not exist.
CMake Error at CMakeLists.txt:1181 (configure_file):
  configure_file Problem configuring file


-- Disabling zlib-ng tests because shared libraries are enabled
CMake Error at CMakeLists.txt:1242 (add_subdirectory):
  add_subdirectory given source "test" which is not an existing directory.

@dzenanz
Copy link
Member

dzenanz commented Jul 10, 2023

If you change tag to something before eace956, e.g. 1b5908b, and run the update script, does the problem still persist? The problem you are running into now might not be related to the rpath fix.

@kislinsk
Copy link
Contributor Author

I set your updateZlib branch as GIT_TAG in our build for ITK and it compiles without any tag changes in the update script. However, when checking our binaries with otool I noticed that the @rpath is wrongly assembled with an absolute path:

...
@rpath/libITKMetaIO-5.4.1.dylib
@rpath//Users/kislinsk/MITK-superbuild/ep/lib/libitkzlib-5.4.1.dylib
...

@dzenanz
Copy link
Member

dzenanz commented Jul 11, 2023

@kislinsk does #4093 fix this?

Unfortunately it introduces CMake errors:

-- ZLIB_HEADER_VERSION: 1.2.13
-- ZLIBNG_HEADER_VERSION: 2.1.3
-- Arch detected: 'x86_64'
-- Basearch of 'x86_64' has been detected as: 'x86'
-- Architecture-specific source files: arch/x86/x86_features.c;arch/x86/slide_hash_avx2.c;arch/x86/chunkset_avx2.c;arch/x86/compare256_avx2.c;arch/x86/adler32_avx2.c;arch/x86/adler32_avx512.c;arch/x86/adler32_avx512_vnni.c;arch/x86/adler32_sse42.c;arch/x86/insert_string_sse42.c;arch/x86/chunkset_sse2.c;arch/x86/compare256_sse2.c;arch/x86/slide_hash_sse2.c;arch/x86/adler32_ssse3.c;arch/x86/chunkset_ssse3.c;arch/x86/crc32_pclmulqdq.c;arch/x86/crc32_vpclmulqdq.c
CMake Error at CMakeLists.txt:919 (file):
  file STRINGS file
  "/Users/kislinsk/MITK-superbuild/ep/src/ITK/zconf-ng.h.in" cannot be read.
Call Stack (most recent call first):
  CMakeLists.txt:930 (generate_cmakein)


CMake Error: File /Users/kislinsk/MITK-superbuild/ep/src/ITK-build/zconf-ng.h.cmakein does not exist.
CMake Error at CMakeLists.txt:1179 (configure_file):
  configure_file Problem configuring file


CMake Error: File /Users/kislinsk/MITK-superbuild/ep/src/ITK/zlib-ng.h.in does not exist.
CMake Error at CMakeLists.txt:1181 (configure_file):
  configure_file Problem configuring file


-- Disabling zlib-ng tests because shared libraries are enabled
CMake Error at CMakeLists.txt:1242 (add_subdirectory):
  add_subdirectory given source "test" which is not an existing directory.

Is this with changes from MITK@a084654 present? If so, does it help to revert that before applying this? If your remove-patch was not included, I think it would be helpful to describe the problem in zlib-ng/zlib-ng#1523, or just link there your comment from here.

@kislinsk
Copy link
Contributor Author

No, this is without my original fix (which works for us BTW :) ). It is applied through a fork of ITK that we clone for patched versions if necessary in our ExternalProject_Add() call. When trying out your branch, I completely changed the GIT_REPOSITORY and GIT_TAG. We also automatically clean the previous build of ITK when any parameters changed so I can guarantee it was a clean build without any changes from our side.

I am not the only one with this issue: zlib-ng/zlib-ng#1523

@dzenanz
Copy link
Member

dzenanz commented Aug 1, 2023

I guess we can close this once zlib-ng/zlib-ng#1546 is merged and zlib updated in ITK.

@dzenanz
Copy link
Member

dzenanz commented Aug 7, 2023

PR open: #4141.

@dzenanz
Copy link
Member

dzenanz commented Aug 7, 2023

@kislinsk does this fix the problem for you?

@kislinsk
Copy link
Contributor Author

kislinsk commented Aug 8, 2023

@kislinsk does this fix the problem for you?

Yes, thank you, it fixes the problem.

@dzenanz dzenanz closed this as completed Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Compiler Compiler support or related warnings
Projects
None yet
Development

No branches or pull requests

3 participants