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

Always symlink TARGET_{LINKER,SONAME}_FILE on libraries #535

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

Conversation

cottsay
Copy link
Contributor

@cottsay cottsay commented Jun 27, 2024

During non-symlink install, these files are copied to the install prefix by default. We should do the same for symlink installs if they're specified.

Reproducer:

  1. $ git clone https://gist.github.com/4d4ebcbd310e6337527e3126ac515298.git src/testpkg
  2. $ colcon build --packages-select testpkg --event-handler console_direct+

Expected:

...
-- Install configuration: ""
-- Execute custom install script
-- Symlinking: /home/cottsay/rolling_ws/install/testpkg/libtestpkg.so.1.2.3
-- Symlinking: /home/cottsay/rolling_ws/install/testpkg/libtestpkg.so.1
-- Symlinking: /home/cottsay/rolling_ws/install/testpkg/libtestpkg.so
-- Symlinking: /home/cottsay/rolling_ws/install/testpkg/libtestpkg.a
-- Symlinking: /home/cottsay/rolling_ws/install/testpkg/testpkg
...

Actual:

...
-- Install configuration: ""
-- Execute custom install script
-- Up-to-date symlink: /home/cottsay/rolling_ws/install/testpkg//libtestpkg.so.1.2.3
-- Up-to-date symlink: /home/cottsay/rolling_ws/install/testpkg//libtestpkg.a
-- Up-to-date symlink: /home/cottsay/rolling_ws/install/testpkg//testpkg
...

Non-symlink:

...
-- Install configuration: ""
-- Installing: /home/cottsay/rolling_ws/install/testpkg/lib/libtestpkg.so.1.2.3
-- Installing: /home/cottsay/rolling_ws/install/testpkg/lib/libtestpkg.so.1
-- Installing: /home/cottsay/rolling_ws/install/testpkg/lib/libtestpkg.so
-- Installing: /home/cottsay/rolling_ws/install/testpkg/lib/libtestpkg.a
-- Installing: /home/cottsay/rolling_ws/install/testpkg/bin/testpkg
-- Set runtime path of "/home/cottsay/rolling_ws/install/testpkg/bin/testpkg" to ""
...

Aside:
I'm not quite to the bottom of it, but installing with EXPORT seems to deactivate the symlink process for targets, which is probably why this bug went unnoticed for so long.

During non-symlink install, these files are copied to the install prefix
by default. We should do the same for symlink installs if they're
specified.

Signed-off-by: Scott K Logan <[email protected]>
@cottsay cottsay added the bug Something isn't working label Jun 27, 2024
@cottsay cottsay self-assigned this Jun 27, 2024
@sloretz
Copy link
Contributor

sloretz commented Jul 19, 2024

I'm not quite to the bottom of it, but installing with EXPORT seems to deactivate the symlink process for targets, which is probably why this bug went unnoticed for so long.

The code for symlink install falls back to the built in _install command when the EXPORT keyword (and some others) are used.

@sloretz
Copy link
Contributor

sloretz commented Jul 19, 2024

It looks like whether these extra files get installed depends on the NAMELINK_ prefixed arguments to install(): https://cmake.org/cmake/help/latest/command/install.html#signatures

What do you think about deferring to the native install command if any of the NAMELINK_ arguments are given?

EDIT: looks like we already do that:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants