gdraheim / zziplib

The ZZIPlib provides read access on ZIP-archives and unpacked data. It features an additional simplified API following the standard Posix API for file access
Other
62 stars 50 forks source link

zzip/CMakeLists.txt: refactor symlink creation #113

Closed veprbl closed 3 years ago

veprbl commented 3 years ago

This hopefully will fix the silent issues that may happen with the install(CODE "execute_process approach.

moritzbuhl commented 3 years ago

I would appreciate this change as it gets rid of the calls to bash which is not installed by default on my platform.

gdraheim commented 3 years ago

@moritzbuhl - I had checked the diff and I found some changes that may make for a different behaviour. I do really like the idea and I want to integrate the patch but I would like to run some tests before. There are parts in the testsuite which can check the identity of the install result. Please allow me some time.

moritzbuhl commented 3 years ago

To my understanding the cmake scripts are not yet fully functional, e.g. the BUILD_STATIC_LIBS option does not work for me. Turning BUILD_SHARED_LIBS off instead does the trick but the previous build system provided shared objects and archives at the same time. As well as libtool files. Should I propose changes based on this pull request?

gdraheim commented 3 years ago
CMake Error at zzip/CMakeLists.txt:290 (install):
   install FILES given no DESTINATION!
CMake Error at zzip/CMakeLists.txt:312 (install):
   install FILES given no DESTINATION!
CMake Error at zzip/CMakeLists.txt:323 (install):
   install FILES given no DESTINATION!
CMake Error at zzip/CMakeLists.txt:337 (install):
   install FILES given no DESTINATION!

Please enable docker and run "make testbuilds" to see the problems with the patch. Actually, I did want to see the difference-tests in the 9xx regions but those are not relevant after the first problems with the symlink-libs installs.

veprbl commented 3 years ago

@gdraheim I don't have an easy access to docker, but this should do the trick.

gdraheim commented 3 years ago

@veprbl it looks much better but a few things are different now.

(a) We have libzzip and libzzipwrap correct using

libzzip-0.so -> libzzip-0.so.13
libzzip-0.so.13.0.72
libzzip.so -> libzzip-0.so.13.0.72

however the non-number dot-so is missing for libzzipmmapped and libzzipfseeko (which did exist before)

 libzzipfseeko-0.so -> libzzipfseeko-0.so.13
 libzzipfseeko-0.so.13.0.72
 libzzipmmapped-0.so -> libzzipmmapped-0.so.13
 libzzipmmapped-0.so.13.0.72

(b) the symlinks exist as required by their value is different

 # OLD
 libzzip-0.so.10 -> libzzip-0.so.13
 libzzip-0.so.11 -> libzzip-0.so.13
 libzzip-0.so.12 -> libzzip-0.so.13
 libzzip-0.so.13 -> libzzip-0.so.13.0.72
 libzzip-0.so.13.0.72
 # NOW
 libzzip-0.so.10 -> libzzip-0.so.13.0.72
 libzzip-0.so.11 -> libzzip-0.so.13.0.72
 libzzip-0.so.12 -> libzzip-0.so.13.0.72
 libzzip-0.so.13 -> libzzip-0.so.13.0.72
 libzzip-0.so.13.0.72

I dont' see that one as a blocker but if you know how to fix it then I would like to see it aligned.

gdraheim commented 3 years ago

Although the latest patch does look correct it does not change the test result. I will check that later (may be it is a problem of the testsuite)

gdraheim commented 3 years ago

It works as discussed - thanks a lot. :)

veprbl commented 3 years ago

To address your (b) point, it was intentional to use the more robust and explicit $<TARGET_FILE_NAME: mechanizm instead of preserving the old behaviour. It should not be too hard to change that.

gdraheim commented 3 years ago

Aye, I did recognize that, so I don't intend to change it.