twogood / unshield

Tool and library to extract CAB files from InstallShield installers
MIT License
351 stars 75 forks source link

Remove md5 and convert_utf from export set when installing #177

Closed bwrsandman closed 11 months ago

bwrsandman commented 11 months ago

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

All good now @bwrsandman ?

kratz00 commented 11 months ago

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

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

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

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

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

Why do we need/build _libconvertutf.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):

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:

_libconvertutf.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 :smile:

bwrsandman commented 11 months ago

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

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

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)
bwrsandman commented 11 months ago

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

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

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.

twogood commented 11 months ago

Let's try this :) And happy holidays!