Skip to content

Skip subdirectories in fix_apple_shared_install_name - #12732

Merged
memsharded merged 3 commits into
conan-io:developfrom
jcar87:bugfix/fix-apple-shared-install-name-bin-subdirs
Dec 19, 2022
Merged

memsharded merged 3 commits into
conan-io:developfrom
jcar87:bugfix/fix-apple-shared-install-name-bin-subdirs

Conversation

@jcar87

@jcar87 jcar87 commented Dec 19, 2022

Copy link
Copy Markdown
Contributor

Changelog: Bugfix: Fix issue in fix_apple_shared_install_name when libdirs or bindirs have subfolders.
Docs: Omit

Close: #12727

Co-authored-by: Rubén Rincón Blanco

Co-authored-by: Rubén Rincón Blanco <ruben@rinconblanco.es>
@jcar87
jcar87 requested review from AbrilRBS and czoido December 19, 2022 11:04

@AbrilRBS AbrilRBS left a comment

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.

👍

@czoido czoido added this to the 1.56 milestone Dec 19, 2022

@jcar87 jcar87 left a comment

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.

On second thought, chances are this function shouldn't recursively find files as we would expect libdirs and bindirs to list all directories in the package_info() instead, in case there's actually relevant libraries in those subfolders

@jcar87 jcar87 changed the title Recursively find binaries in fix_apple_shared_install_name Skip subdirectories in fix_apple_shared_install_name Dec 19, 2022
@memsharded

Copy link
Copy Markdown
Member

On second thought, chances are this function shouldn't recursively find files as we would expect libdirs and bindirs to list all directories in the package_info() instead, in case there's actually relevant libraries in those subfolders

It is also important to consider that consumers via VirtualXXXEnv will add the bindirs path to the PATH env-var in order to find executables and DYLD_LIBRARY_PATH for shared libraries. If libraries are in nested subfolders, they will not be found, so it seems those folders should be listed in bindirs/libdirs?

Comment thread conan/tools/apple/apple.py Outdated
Co-authored-by: Francisco Ramírez <franchuti688@gmail.com>
if binary_type not in ("DYLIB", "EXECUTE") or os.path.islink(file) or os.path.isdir(file):
return False
check_file = f"otool -hv {file}"
return binary_type in check_output_runner(check_file)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suppose check_output_runner may raise, therefore any error from otool invocation (like, it failed to parse a particular file, or just missing), will result in failure to create a package.
should it have try/catch block here to return False in case of any errors?

@memsharded
memsharded merged commit 157202f into conan-io:develop Dec 19, 2022
@jcar87
jcar87 deleted the bugfix/fix-apple-shared-install-name-bin-subdirs branch December 19, 2022 17:09
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.

[bug] fix_apple_shared_install_name() fails if there is a subfolder in bin folder

6 participants