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 installation of shared library symlink #535

Merged
merged 1 commit into from Mar 20, 2024

Conversation

yuyichao
Copy link
Contributor

@yuyichao yuyichao commented Mar 19, 2024

This partially reverts 29391cc.

The dummy target that was removed in 29391cc was responsible for installing the unversioned symlink for the shared libraries. The only part of this that was causing issue appears to be the dummy destination that was only added as an workaround for old cmake version (pre 3.14-ish). With the cmake version requirement bumped to 3.18 this should not be an issue anymore.

This also fixes a typo from eb3d977 causing the libsleefgnuabi library to be missing the symlink...

Fixes #532

@yuyichao
Copy link
Contributor Author

And to save ppl some time digging through this again. These targets were originally added in eb3d977 to split out the library itself (which is required at runtime) and the namelink (which is required at compile/link time only) into different components. (This commit also introduced a typo that caused the gnueabi library to be missing the link unfortunately...)

There appears to be some compatibility issues with old cmake versions (#417) as noted and fixed in #419 though I couldn't really find the reference for the quote in that PR and a brief looking at the document for pre-cmake 3.12/3.14 doesn't make it immediately obvious to me why the dummy default DESTINATION is required.

The commit message for the removal of the dummy target doesn't have anything useful in it but based on the comments in #531 (comment) and #510 I'm guessing it's only about the installation to "dummy" without realizing it's also removing the namelink needed for linking so I assume adding back the target itself shouldn't be a problem.

@blapie
Copy link
Collaborator

blapie commented Mar 19, 2024

Hi! Thanks a lot for looking into that and explaining in details.

I cannot see what the mentioned typo was from your patch? Can you point me to it?

I don't think there was a particular use for the dummy target, outside what you mentioned. Maybe @friendlyanon can correct me, advise and confirm that it was only there for compatibility with old cmake.

The justification for removing the dummy target was mostly cosmetic, I believe. If with the new minimum required came version of 3.18 and your patch we can avoid it and bring back the unversioned symlink altogether, then I think it is a win.

Otherwise change sounds good to me, will test locally.
We will make sure to set up a few extra tests to avoid this being missed in the future.

@yuyichao
Copy link
Contributor Author

eb3d977#diff-888e45d79814842a399e05bff360c2598a83fdcdffd2c3fcb66f8ebf3d4dd86dR1097

The target name here was wrong. Probably from copy and pasting from a few lines above in the same file…

@friendlyanon
Copy link
Contributor

The dummy destination for the NAMELINK_ONLY rule was there to support older CMake releases that the project claimed to support via cmake_minimum_required, because those versions require the destination value to be filled, even if they go unused in the rule.

Since the project promises 3.18 support now, NAMELINK_{SKIP,ONLY} should be replaced with NAMELINK_COMPONENT which does the install component separation in a simpler way, without requiring 2 separate install rules: https://github.com/friendlyanon/cxx-static-shared-example?tab=readme-ov-file#name-link

This fixes the regression from 29391cc.

The dummy target that was removed in 29391cc
was responsible for installing the unversioned symlink for the shared libraries.
The only part of this that was causing issue appears to be the dummy destination
that was only added as an workaround for old cmake version (pre 3.14-ish).
With the cmake version requirement bumped to 3.18 this should not be an issue anymore,
and we could also use the new NAMELINK_COMPONENT option to avoid using two install
statements.

This also fixes a bug from eb3d977 causing
the libsleefgnuabi library to be missing the symlink...
@yuyichao
Copy link
Contributor Author

NAMELINK_{SKIP,ONLY} should be replaced with NAMELINK_COMPONENT

Done.

Copy link
Contributor

@friendlyanon friendlyanon left a comment

Choose a reason for hiding this comment

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

I can't verify right now if these are all the instances, but otherwise LGTM.

@blapie
Copy link
Collaborator

blapie commented Mar 19, 2024

Looks good to me too. I believe these are all the targets.

Is it what is expected when installing each components? If so then I'll merge.

$> cat build/install_manifest_sleef_Runtime.txt 
/home/piebla01/sleef/sleef/install/lib/libsleefscalar.so.3.6.0
/home/piebla01/sleef/sleef/install/lib/libsleefscalar.so.3
/home/piebla01/sleef/sleef/install/lib/libsleef.so.3.6.0
/home/piebla01/sleef/sleef/install/lib/libsleef.so.3
/home/piebla01/sleef/sleef/install/lib/libsleefgnuabi.so.3.6
/home/piebla01/sleef/sleef/install/lib/libsleefgnuabi.so.3
/home/piebla01/sleef/sleef/install/lib/libsleefdft.so.3.6.0
/home/piebla01/sleef/sleef/install/lib/libsleefdft.so.3
/home/piebla01/sleef/sleef/install/lib/libsleefquad.so.3.6.0
/home/piebla01/sleef/sleef/install/lib/libsleefquad.so.3

and

$> cat build/install_manifest_sleef_Development.txt
/home/piebla01/sleef/sleef/install/lib/libsleefscalar.so
/home/piebla01/sleef/sleef/install/lib/libsleef.so
/home/piebla01/sleef/sleef/install/include/sleef.h
/home/piebla01/sleef/sleef/install/lib/pkgconfig/sleef.pc
/home/piebla01/sleef/sleef/install/lib/libsleefgnuabi.so
/home/piebla01/sleef/sleef/install/lib/libsleefdft.so
/home/piebla01/sleef/sleef/install/include/sleefdft.h
/home/piebla01/sleef/sleef/install/lib/libsleefquad.so
/home/piebla01/sleef/sleef/install/include/sleefquad.h
/home/piebla01/sleef/sleef/install/lib/cmake/sleef/sleefConfig.cmake
/home/piebla01/sleef/sleef/install/lib/cmake/sleef/sleefConfigVersion.cmake
/home/piebla01/sleef/sleef/install/lib/cmake/sleef/sleefTargets.cmake
/home/piebla01/sleef/sleef/install/lib/cmake/sleef/sleefTargets-release.cmake

I don't think a GNU ABI header is required.

@friendlyanon
Copy link
Contributor

Yep, the runtime component should really only have the real library and the soname, everything else goes in the dev component. Think foo and foo-dev packages in the APT world.

@blapie
Copy link
Collaborator

blapie commented Mar 19, 2024

And should we expect the symlink to show in red in the dev component?

libsleef.so -> libsleef.so.3
libsleefdft.so -> libsleefdft.so.3
libsleefgnuabi.so -> libsleefgnuabi.so.3
libsleefquad.so -> libsleefquad.so.3
libsleefscalar.so -> libsleefscalar.so.3

All show in red.

@friendlyanon
Copy link
Contributor

friendlyanon commented Mar 19, 2024

I don't remember what a red name means with ls --color, but CMake does the right thing here.

@yuyichao
Copy link
Contributor Author

yuyichao commented Mar 19, 2024

The red means that the link target isn't there, which is expected since you didn't install the runtime library in the same directory. At the end of the day when the dev component is installed (as a separate package) it must still be installed to the same directory as the runtime library and the symlink would work.

@musicinmybrain
Copy link
Contributor

$ git clean -fxd
$ cmake -S. -Bbuild -GNinja -DSLEEF_BUILD_TESTS:BOOL=OFF -DSLEEF_BUILD_SHARED_LIBS:BOOL=ON -DSLEEF_BUILD_SCALAR_LIB:BOOL=ON -DSLEEF_BUILD_DFT:BOOL=ON -DSLEEF_BUILD_QUAD:BOOL=ON
$ cmake --build build -j16
$ mkdir foo
$ DESTDIR="${PWD}/foo" cmake --install build
$ ls -l foo/usr/local/lib64/
total 9736
drwxr-xr-x. 1 ben ben      10 Mar 19 17:22 cmake
lrwxrwxrwx. 1 ben ben      16 Mar 19 17:22 libsleefdft.so -> libsleefdft.so.3
lrwxrwxrwx. 1 ben ben      20 Mar 19 17:22 libsleefdft.so.3 -> libsleefdft.so.3.6.0
-rwxr-xr-x. 1 ben ben 4432632 Mar 19 17:22 libsleefdft.so.3.6.0
lrwxrwxrwx. 1 ben ben      19 Mar 19 17:22 libsleefgnuabi.so -> libsleefgnuabi.so.3
lrwxrwxrwx. 1 ben ben      21 Mar 19 17:22 libsleefgnuabi.so.3 -> libsleefgnuabi.so.3.6
-rwxr-xr-x. 1 ben ben  700416 Mar 19 17:22 libsleefgnuabi.so.3.6
lrwxrwxrwx. 1 ben ben      17 Mar 19 17:22 libsleefquad.so -> libsleefquad.so.3
lrwxrwxrwx. 1 ben ben      21 Mar 19 17:22 libsleefquad.so.3 -> libsleefquad.so.3.6.0
-rwxr-xr-x. 1 ben ben 2097576 Mar 19 17:22 libsleefquad.so.3.6.0
lrwxrwxrwx. 1 ben ben      19 Mar 19 17:22 libsleefscalar.so -> libsleefscalar.so.3
lrwxrwxrwx. 1 ben ben      23 Mar 19 17:22 libsleefscalar.so.3 -> libsleefscalar.so.3.6.0
-rwxr-xr-x. 1 ben ben  210328 Mar 19 17:22 libsleefscalar.so.3.6.0
lrwxrwxrwx. 1 ben ben      13 Mar 19 17:22 libsleef.so -> libsleef.so.3
lrwxrwxrwx. 1 ben ben      17 Mar 19 17:22 libsleef.so.3 -> libsleef.so.3.6.0
-rwxr-xr-x. 1 ben ben 2476856 Mar 19 17:22 libsleef.so.3.6.0
drwxr-xr-x. 1 ben ben      16 Mar 19 17:22 pkgconfig

This looks right!

@musicinmybrain
Copy link
Contributor

I also tried using this as a patch with the 3.6 tag and confirmed that it fixes #532.

@yuyichao
Copy link
Contributor Author

Ah, I didn't realize that there was an issue for this. I just looked at the open PR's but not the issues. #532 was basically the typo that I mentioned above.

@blapie
Copy link
Collaborator

blapie commented Mar 20, 2024

At the end of the day when the dev component is installed (as a separate package) it must still be installed to the same directory as the runtime library and the symlink would work.

Ok, that's what I was suspecting. Thanks for clarifying!

@blapie blapie merged commit 60e76d2 into shibatch:master Mar 20, 2024
31 checks passed
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.

Missing unversioned symbolic link for GNUABI version in 3.6
4 participants