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

Fix runtime linking on Mac OS when SIP enabled #212

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Nov 30, 2019

This makes runtime dynamic linking use the same paths as compile-time, which is correct enough to make rclpy work on a Mac when SIP (System Integrity Protection) is enabled. This may not be sufficient when an underlay library wants to dynamically link to a library in an overlay; e.g. installing an RMW implementation in an overlay that is not present in the underlay.

Here's an example of an error this fixes:

15: test_subscriber_terminates_in_a_finite_amount_of_time[BoundedSequences] (test_communication.TestPublisherSubscriber)
15: Test that the subscriber terminates after a finite amount of time. ... [INFO] [test_publisher-5]: process started with pid [46399]
15: [INFO] [test_subscriber-6]: process started with pid [46400]
15: [test_publisher-5] exception in talker:
15: [test_publisher-5] Traceback (most recent call last):
15: [test_publisher-5]   File "/opt/ros/master/src/ros2/system_tests/test_communication/test/publisher_py.py", line 63, in <module>
15: [test_publisher-5]     number_of_cycles=args.number_of_cycles)
15: [test_publisher-5]   File "/opt/ros/master/src/ros2/system_tests/test_communication/test/publisher_py.py", line 29, in talker
15: [test_publisher-5]     rclpy.init(args=[])
15: [test_publisher-5]   File "/opt/ros/master/install/lib/python3.7/site-packages/rclpy/__init__.py", line 70, in init
15: [test_publisher-5]     context = get_default_context() if context is None else context
15: [test_publisher-5]   File "/opt/ros/master/install/lib/python3.7/site-packages/rclpy/utilities.py", line 31, in get_default_context
15: [test_publisher-5]     g_default_context = Context()
15: [test_publisher-5]   File "/opt/ros/master/install/lib/python3.7/site-packages/rclpy/context.py", line 30, in __init__
15: [test_publisher-5]     from rclpy.impl.implementation_singleton import rclpy_implementation
15: [test_publisher-5]   File "/opt/ros/master/install/lib/python3.7/site-packages/rclpy/impl/implementation_singleton.py", line 31, in <module>
15: [test_publisher-5]     rclpy_implementation = _import('._rclpy')
15: [test_publisher-5]   File "/opt/ros/master/install/lib/python3.7/site-packages/rclpy/impl/__init__.py", line 21, in _import
15: [test_publisher-5]     return importlib.import_module(name, package='rclpy')
15: [test_publisher-5]   File "/usr/local/Cellar/python/3.7.5/Frameworks/Python.framework/Versions/3.7/lib/python3.7/importlib/__init__.py", line 127, in import_module
15: [test_publisher-5]     return _bootstrap._gcd_import(name[level:], package, level)
15: [test_publisher-5] ImportError: dlopen(/opt/ros/master/install/lib/python3.7/site-packages/rclpy/_rclpy.cpython-37m-darwin.so, 2): Library not loaded: @rpath/librclpy_common.dylib
15: [test_publisher-5]   Referenced from: /opt/ros/master/install/lib/python3.7/site-packages/rclpy/_rclpy.cpython-37m-darwin.so
15: [test_publisher-5]   Reason: image not found
15: [test_publisher-5] The C extension '/opt/ros/master/install/lib/python3.7/site-packages/rclpy/_rclpy.cpython-37m-darwin.so' failed to be imported while being present on the system. Please refer to 'https://index.ros.org/doc/ros2/Troubleshooting/#import-failing-even-with-library-present-on-the-system' for possible solutions

Signed-off-by: Dan Rose dan@digilabs.io

Signed-off-by: Dan Rose <dan@digilabs.io>
@dirk-thomas
Copy link
Contributor

Someone from the @ament/team with more macOS experience might want to comment here.

I am skeptical that globally keeping an RPATH is the right approach.

@wjwwood
Copy link
Contributor

wjwwood commented Jan 23, 2020

I think we would not take this patch as-is unless we could have a few things answered:

  • does this break relocatability?
  • does this mean we could avoid setting DYLD_LIBRARY_PATH all together?

Until someone has time to really investigate this and figure out the drawbacks, if any, or show that it's safe/correct to do this we will put it in the backlog.

I will leave it open however, in case someone wants to cherry-pick it for their own development.

@wjwwood wjwwood removed their assignment Jan 23, 2020
@wjwwood wjwwood added backlog help wanted Extra attention is needed labels Jan 23, 2020
@Karsten1987
Copy link
Contributor

I am not sure how deeply this is related to this problem.

After disabling SIP, the compilation of my ROS2 workspace turned out to be extremely long. The main reason for this is colcon/ament concatenating the DYLD_LIBRARY_PATH which resulted in a very long linking time.
In order to overcome this, I've disabled the library path hooks in colcon as well as ament which sped up my compilation time by a lot.
When sourcing my workspace afterwards, my applications and such work correctly. What is not going to work is dynamically loading components as the respective DYLD_LIBRARY_PATH is not set. That behavior is expected though and might be related to this. When manually extending the library path, also the (composition) demos work correctly. Similarly for tests, when running colcon test the workspace in question has to be sourced beforehand. All this works without the proposed change here, but might also be related to the fact, that I have SIP disabled.

This all is to say, that this behavior is not occurring when SIP is enabled as the DYLD_LIBRARY_PATH won't be extended in the first place. But moreover, with SIP being enabled, things like colcon test aren't working correctly anyway.

@audrow audrow changed the base branch from master to rolling June 28, 2022 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants