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

WIP: [libzippp] New port: C++ wrapper for libzip #6801

Merged
merged 5 commits into from
Jul 22, 2019

Conversation

PhilLab
Copy link
Contributor

@PhilLab PhilLab commented Jun 6, 2019

Closes #6600

Please review thoroughly as this was more or less trial-and-error :-D

@PhilLab PhilLab changed the title [libzippp] New port: C++ wrapper for libzip WIP: [libzippp] New port: C++ wrapper for libzip Jun 6, 2019
@PhilLab
Copy link
Contributor Author

PhilLab commented Jun 7, 2019

@ctabin are you interested in the vcpkg port? The windows-static port fails and Linux / OSX are still missing

@ctabin
Copy link
Contributor

ctabin commented Jun 7, 2019

Hello @PhilLab,

Yes that would be great !
Let me know how I can help.

@PhilLab
Copy link
Contributor Author

PhilLab commented Jun 12, 2019

@ctabin: Could help with the static port? I don't quite understand why it fails

@ctabin
Copy link
Contributor

ctabin commented Jun 13, 2019

@PhilLab I'll take a look asap 😅

ports/libzippp/portfile.cmake Outdated Show resolved Hide resolved
@vicroms
Copy link
Member

vicroms commented Jun 13, 2019

Hi @PhilLab

Thanks for the enthusiasm in adding new ports!

I made some (pretty invasive) changes to your PR.

I replaced the build system (CMakeLists.txt file), to address some issues (@ctabin can you review whether these changes are wanted, so I might consider submitting a PR to libzippp's repository):

  • Respect BUILD_SHARED_LIBS variable.
    • Allows users to select whether to build STATIC or SHARED libraries from the command line, or in vcpkg by using an appropriate triplet.
    • Removes -shared and -static suffixes from produced binaries. A static build produces lib/libzippp.lib, and a shared build produces bin/libzippp.dll, bin/libzippp.pdb and lib/libzippp.lib.
    • NOTE: This change can break downstream consumers that rely on the old names, possibly alleviated by the next change.
  • Support CMake integration using find_package(libzippp::libzippp)
    • Have CMake generate libzipppConfig.cmake and libzipppTargets*.cmake.
  • Use a FindLIBZIP.cmake module to search for libzip.
    • Removes reliance on hard-coded paths.**
  • Allow CMake on MacOS and Linux.
  • Add option BUILD_TESTS to enable skipping building of tests
    • BUILD_TESTS is ON by default.

@PhilLab
Copy link
Contributor Author

PhilLab commented Jun 14, 2019

@vicroms: Thank you for extending the PR. As my current Cmake knowledge is limited to "making stuff work somehow" it is good to see a clearer approach 😄

@ctabin
Copy link
Contributor

ctabin commented Jun 14, 2019

Hello @vicroms,

Thanks for investigating and solving the static builds ! 😃 Your changes looks good to me and it would be awesome if you could submit a PR, so I would to a new release of libzippp with the latest changes.

@vicroms
Copy link
Member

vicroms commented Jul 22, 2019

Thanks @PhilLab and @ctabin for this PR!
The port is now clean of extra CMake files and works with the upstream CMakeLists.txt!

@vicroms vicroms merged commit 042f964 into microsoft:master Jul 22, 2019
@PhilLab
Copy link
Contributor Author

PhilLab commented Sep 27, 2019

@vicroms The port files now do not copy the pdb files anymore, is this intended?

@PhilLab
Copy link
Contributor Author

PhilLab commented Oct 1, 2019

@vicroms Additionally as I noticed now, the dependent libzip module does not come with cmake support.

TARGET_LINK_LIBRARIES(myTarget LIBZIPPP::LIBZIPPP)

fails because Libzip could not be found:

Could not find a package configuration file provided by "LIBZIP" with any
of the following names:

LIBZIPConfig.cmake
libzip-config.cmake

@Flamefire
Copy link

@PhilLab Unfortunately LibZip does not provide CMake support for the installed targets. I'm working on ctabin/libzippp#52 to install the FindLIBZIP.cmake (which fixes that) together with some other changes. You might want to watch that PR and update the vcpkg once merged

@PhilLab PhilLab deleted the libzippp branch November 26, 2019 14:16
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.

libzippp: C++ wrapper for libzip
5 participants