shibatch / sleef

SIMD Library for Evaluating Elementary Functions, vectorized libm and DFT
https://sleef.org
Boost Software License 1.0
628 stars 126 forks source link

Fix installation of shared library symlink #535

Closed yuyichao closed 5 months ago

yuyichao commented 5 months ago

This partially reverts 29391ccd9d513132fc802a198f8b801fb273cb59.

The dummy target that was removed in 29391ccd9d513132fc802a198f8b801fb273cb59 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 eb3d97785cb4f6f6f4c0ffd6570ca6c73ccac2ec causing the libsleefgnuabi library to be missing the symlink...

Fixes https://github.com/shibatch/sleef/issues/532

yuyichao commented 5 months ago

And to save ppl some time digging through this again. These targets were originally added in eb3d97785cb4f6f6f4c0ffd6570ca6c73ccac2ec 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 https://github.com/shibatch/sleef/pull/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 https://github.com/shibatch/sleef/pull/531#issuecomment-2002208808 and https://github.com/shibatch/sleef/pull/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 commented 5 months ago

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 commented 5 months ago

https://github.com/shibatch/sleef/commit/eb3d97785cb4f6f6f4c0ffd6570ca6c73ccac2ec#diff-888e45d79814842a399e05bff360c2598a83fdcdffd2c3fcb66f8ebf3d4dd86dR1097

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

friendlyanon commented 5 months ago

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

yuyichao commented 5 months ago

NAMELINK_{SKIP,ONLY} should be replaced with NAMELINK_COMPONENT

Done.

blapie commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

yuyichao commented 5 months ago

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 commented 5 months ago
$ 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 commented 5 months ago

I also tried using this as a patch with the 3.6 tag and confirmed that it fixes https://github.com/shibatch/sleef/issues/532.

yuyichao commented 5 months ago

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 commented 5 months ago

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!