Remove md5 and convert_utf from export set when installing #177

bwrsandman · 2023-12-12T20:07:05Z

In #164, I neglected to test on a system without an md5 install, not knowing the mechanism of USE_OWN_MD5.

This PR fixes the issue of getting this error about the export sets

CMake Error: install(EXPORT "unshieldConfig" ...) includes target "libunshield" which requires target "md5" that is not in any export set.

There is also an issue of too many static libs being installed.

twogood · 2023-12-14T20:34:43Z

All good now @bwrsandman ?

kratz00 · 2023-12-14T21:44:00Z

I have one question. with this change header files are installed too now, which makes total sense for libunshield.h.
Does it for

cabfile.h
internal.h
log.h

though? It feels like these are all internal headers. What do you think?

Unrelated to this PR but your changes makes me wounder if the pkg-config file (https://github.com/twogood/unshield/blob/main/libunshield.pc.in) is not completely broken if build statically, as it does not mention libconvert_utf.a nor libmd5.a in this case.

bwrsandman · 2023-12-15T00:15:12Z

I had the same question about the headers. Additionally do you expect them in /usr/include/unshield/
As for the static builds, I'll admit I'm not very familiar with how static libraries get distributed and why cmake insists on distributing even private dependencies.

twogood · 2023-12-15T07:26:22Z

The purpose of the static build is for the unshield binary, you know like Go applications. A single file that can be scp:ed between systems and whatnot.

But of course we could probably have a statically linkable linunshield.a as well, but it's not the priority.

twogood · 2023-12-15T13:01:57Z

I have one question. with this change header files are installed too now, which makes total sense for libunshield.h. Does it for

cabfile.h

internal.h

log.h

though? It feels like these are all internal headers. What do you think?

These must not be installed!

kratz00 · 2023-12-15T14:16:46Z

Why do we need/build libconvert_utf.a or libmd5.a in the first place?
In case of static linking we end up with three static libraries (if USE_OUR_OWN_MD5 is ON):

libconvert_utf.a
libmd5.a
libunshield.a

In case of dynamic linking we have one shared library (plus some symbolic links to it) as the object files from the static libraries just end up in the shared object file:

libunshield.so@
libunshield.so.1@
libunshield.so.1.5.1*

libconvert_utf.a and libmd5.a are always build as static libraries, see https://github.com/twogood/unshield/blob/main/lib/convert_utf/CMakeLists.txt#L13 and here https://github.com/twogood/unshield/blob/main/lib/md5/CMakeLists.txt

I think there is no need to build those two as independent libraries and just included the object files in libunshield,
which would make distribution easier. I can create a pull request to implement the idea if there are no objections 😄

bwrsandman · 2023-12-15T16:44:36Z

Sounds like this should then be the installation and please correct me if I'm wrong:

bin/unshield
lib/[lib]unshield{.dylib,.dll,.so*,.a}
include/unshield.h and not include/unshield/unshield.h

I see the usefulness of have a portable exe, distributing dynamic libs on Linux is more standard and helps save space when packages depend on it. Doing both for a package is less standard but not too crazy.

Packaging for dependency managers and package managers can also deal with how they want to have everything linked through options of the cmake.

Here's what I suggest: Compiling statically and dynamically on github will give us the portable binary, the static and dynamic libs can co-exist. This can done by compiling both with static on and off, combining the results. That way anyone checking the releases, can find pre-compiled binaries and get going fast. I think the only tricky thing is cmake configs.

Then package managers can compile from source and do it how they like.

bwrsandman · 2023-12-15T17:54:40Z

Latest push this is what it looks like when compiled (on mac)
static

.
├── bin
│   └── unshield
├── include
│   └── libunshield.h
├── lib
│   ├── cmake
│   │   └── unshield
│   │       ├── unshieldConfig-noconfig.cmake
│   │       └── unshieldConfig.cmake
│   ├── libconvert_utf.a
│   ├── libunshield.a
│   └── pkgconfig
│       └── libunshield.pc
└── share
    └── man
        └── man1
            └── unshield.1

otool -L bin/unshield lib/libunshield.a lib/libconvert_utf.a 
bin/unshield:
        /usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.12)
        /opt/homebrew/opt/openssl@3/lib/libcrypto.3.dylib (compatibility version 3.0.0, current version 3.0.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1336.61.1)
Archive : lib/libunshield.a
lib/libunshield.a(component.c.o):
lib/libunshield.a(directory.c.o):
lib/libunshield.a(file.c.o):
lib/libunshield.a(file_group.c.o):
lib/libunshield.a(helper.c.o):
lib/libunshield.a(libunshield.c.o):
lib/libunshield.a(log.c.o):
Archive : lib/libconvert_utf.a
lib/libconvert_utf.a(ConvertUTF.c.o):

non-static

.
├── bin
│   └── unshield
├── include
│   └── libunshield.h
├── lib
│   ├── cmake
│   │   └── unshield
│   │       ├── unshieldConfig-noconfig.cmake
│   │       └── unshieldConfig.cmake
│   ├── libunshield.1.5.1.dylib
│   ├── libunshield.1.dylib -> libunshield.1.5.1.dylib
│   ├── libunshield.dylib -> libunshield.1.dylib
│   └── pkgconfig
│       └── libunshield.pc
└── share
    └── man
        └── man1
            └── unshield.1

otool -L bin/unshield lib/libunshield.dylib 
bin/unshield:
        @rpath/libunshield.1.dylib (compatibility version 1.0.0, current version 1.5.1)
        /usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.12)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1336.61.1)
lib/libunshield.dylib:
        @rpath/libunshield.1.dylib (compatibility version 1.0.0, current version 1.5.1)
        /usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.12)
        /opt/homebrew/opt/openssl@3/lib/libcrypto.3.dylib (compatibility version 3.0.0, current version 3.0.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1336.61.1)

…amic

bwrsandman · 2023-12-15T19:37:13Z

Resolved the issue with extra static libraries by using the OBJECT type of library which I believe was the original intent.

This is what the installation looks like now

static

.
├── bin
│   └── unshield
├── include
│   └── libunshield.h
├── lib
│   ├── cmake
│   │   └── unshield
│   │       ├── unshieldConfig-noconfig.cmake
│   │       └── unshieldConfig.cmake
│   ├── libunshield.a
│   └── pkgconfig
│       └── libunshield.pc
└── share
    └── man
        └── man1
            └── unshield.1

otool -L bin/unshield lib/libunshield.a 
bin/unshield:
        /usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.12)
        /opt/homebrew/opt/openssl@3/lib/libcrypto.3.dylib (compatibility version 3.0.0, current version 3.0.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1336.61.1)
Archive : lib/libunshield.a
lib/libunshield.a(component.c.o):
lib/libunshield.a(directory.c.o):
lib/libunshield.a(file.c.o):
lib/libunshield.a(file_group.c.o):
lib/libunshield.a(helper.c.o):
lib/libunshield.a(libunshield.c.o):
lib/libunshield.a(log.c.o):
lib/libunshield.a(ConvertUTF.c.o):

non-static

.
├── bin
│   └── unshield
├── include
│   └── libunshield.h
├── lib
│   ├── cmake
│   │   └── unshield
│   │       ├── unshieldConfig-noconfig.cmake
│   │       └── unshieldConfig.cmake
│   ├── libunshield.1.5.1.dylib
│   ├── libunshield.1.dylib -> libunshield.1.5.1.dylib
│   ├── libunshield.dylib -> libunshield.1.dylib
│   └── pkgconfig
│       └── libunshield.pc
└── share
    └── man
        └── man1
            └── unshield.1

otool -L bin/unshield lib/libunshield.dylib 
bin/unshield:
        @rpath/libunshield.1.dylib (compatibility version 1.0.0, current version 1.5.1)
        /usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.12)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1336.61.1)
lib/libunshield.dylib:
        @rpath/libunshield.1.dylib (compatibility version 1.0.0, current version 1.5.1)
        /usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.12)
        /opt/homebrew/opt/openssl@3/lib/libcrypto.3.dylib (compatibility version 3.0.0, current version 3.0.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1336.61.1)

bwrsandman · 2023-12-16T00:30:30Z

Question: is Zlib a public dependency? By that I mean do headers leak includes from Zlib? Does a program linking dynamically with libunshield.so need to also link with Zlib or use the Zlib dev package?

twogood · 2023-12-16T16:43:56Z

Question: is Zlib a public dependency? By that I mean do headers leak includes from Zlib? Does a program linking dynamically with libunshield.so need to also link with Zlib or use the Zlib dev package?

Yes, if you would link with libunshield you also need to link with zib, either dynamically or statically.

kratz00 · 2023-12-18T15:38:10Z

CMakeLists.txt

@@ -1,4 +1,4 @@
-cmake_minimum_required(VERSION 3.7)
+cmake_minimum_required(VERSION 3.21)


I see the need to increase this, because of https://cmake.org/cmake/help/latest/command/add_library.html#object-libraries
Shouldn't 3.12 be enough?

I did not test it yet, but just from looking at the diff this looks great, thanks @bwrsandman

Using OBJECT is 3.12 but using TARGET_OBJECTS with them is 3.21. This allows us to not export convert_utf and md5 during install.

See the difference in the last commit of the pr.

It's clear now, thanks for the explanation.

twogood · 2023-12-22T18:56:48Z

Let's try this :) And happy holidays!

twogood self-assigned this Dec 14, 2023

bwrsandman force-pushed the patch-2 branch 3 times, most recently from 87001cf to dd27d11 Compare December 15, 2023 19:22

bwrsandman added 4 commits December 15, 2023 14:29

Link md5 as object and privately to avoid needing to export it in dyn…

4dbd460

…amic

Don't export openssl dependency

19237e4

Fix installing headers

9c3bd57

Convert convert_utf to OBJECT lib

d1ab74b

bwrsandman force-pushed the patch-2 branch from dd27d11 to d1ab74b Compare December 15, 2023 19:29

Link with TARGET_OBJECT to avoid exporting OBJECT deps too much

856516b

bwrsandman changed the title ~~Add md5 to export set when compiled STATIC~~ Remove md5 and convert_utf from export set when installing Dec 15, 2023

kratz00 reviewed Dec 18, 2023

View reviewed changes

kratz00 approved these changes Dec 19, 2023

View reviewed changes

twogood merged commit 082df8d into twogood:main Dec 22, 2023

bwrsandman deleted the patch-2 branch December 23, 2023 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove md5 and convert_utf from export set when installing #177

Remove md5 and convert_utf from export set when installing #177

bwrsandman commented Dec 12, 2023 •

edited

Loading

twogood commented Dec 14, 2023

kratz00 commented Dec 14, 2023

bwrsandman commented Dec 15, 2023 •

edited

Loading

twogood commented Dec 15, 2023 •

edited

Loading

twogood commented Dec 15, 2023

kratz00 commented Dec 15, 2023

bwrsandman commented Dec 15, 2023 •

edited

Loading

bwrsandman commented Dec 15, 2023

bwrsandman commented Dec 15, 2023

bwrsandman commented Dec 16, 2023 •

edited

Loading

twogood commented Dec 16, 2023

kratz00 Dec 18, 2023

bwrsandman Dec 18, 2023 •

edited

Loading

kratz00 Dec 19, 2023

twogood commented Dec 22, 2023

		@@ -1,4 +1,4 @@
		cmake_minimum_required(VERSION 3.7)
		cmake_minimum_required(VERSION 3.21)

Remove md5 and convert_utf from export set when installing #177

Remove md5 and convert_utf from export set when installing #177

Conversation

bwrsandman commented Dec 12, 2023 • edited Loading

twogood commented Dec 14, 2023

kratz00 commented Dec 14, 2023

bwrsandman commented Dec 15, 2023 • edited Loading

twogood commented Dec 15, 2023 • edited Loading

twogood commented Dec 15, 2023

kratz00 commented Dec 15, 2023

bwrsandman commented Dec 15, 2023 • edited Loading

bwrsandman commented Dec 15, 2023

bwrsandman commented Dec 15, 2023

bwrsandman commented Dec 16, 2023 • edited Loading

twogood commented Dec 16, 2023

kratz00 Dec 18, 2023

Choose a reason for hiding this comment

bwrsandman Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

kratz00 Dec 19, 2023

Choose a reason for hiding this comment

twogood commented Dec 22, 2023

bwrsandman commented Dec 12, 2023 •

edited

Loading

bwrsandman commented Dec 15, 2023 •

edited

Loading

twogood commented Dec 15, 2023 •

edited

Loading

bwrsandman commented Dec 15, 2023 •

edited

Loading

bwrsandman commented Dec 16, 2023 •

edited

Loading

bwrsandman Dec 18, 2023 •

edited

Loading