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

CMake - proper exports or find module missing, Windows builds not self-contained #27

Closed
thokra1 opened this issue Feb 6, 2018 · 8 comments

Comments

@thokra1
Copy link

thokra1 commented Feb 6, 2018

When we first tried to use libzip with MSVC++ 14.1 (Visual Studio 2017), we stumbled upon the fact that without modifying the build system, a proper build is not possible. The "Packages" page refers to Microsofts huge library porting repository which has some patches and MS-specific CMake stuff to properly automate MSVC builds. In addition, proper generation of config-based libraries with appropriate library suffixes currently isn't implemented (i.e. debug and release build artifacts replace each other).

During install, no import targets are generated, so integration into a CMake-based environment either has to be done manually, by doing all the legwork directly in your projects or by writing proper modules for use with find_package(). Unless I overlooked it, it seems libzip doesn't provide a find module.

The dependency on zlib isn't an issue with CMake, as the proper find module is distributed with CMake.

Now, we could do all of this ourselves and leave it that, but I'd much rather simply use the upstream.

I suggest the following set of fixes:

  • make the library properly build out of the box on Windows (no patching or anything else needed)
  • fix library naming to at least have different outputs for debug and release builds
  • either provide the proper mechanism to export target definitions or add a proper find module to ease integration in other projects (I'd likely just do both)

Would you be interested in a corresponding PR?

@0-wiz-0
Copy link
Member

0-wiz-0 commented Feb 11, 2018

We recently integrated some Windows-specific changes, so please try the latest git version. I'm not aware of any patches needed for building on Windows.

That being said, we're not using Windows ourselves, so we're always happy to integrate improvements for that, as long as they don't break stuff on Unix systems. I'm looking forward to your PR! :)

@thokra1
Copy link
Author

thokra1 commented Feb 12, 2018

First of all, thank you for commenting! I'll have to spread the PRs out a little, i think.

I strongly suggest to add Windows CI using something like AppVeyor - like TravisCI, it's free for OSS. Even if Windows isn't a first-class citizen, this way you'd at least get instant feedback. I'd be happy to do this for you, if you wish (and if you're willing to permit the corresponding github webhooks 😃 ).

Just to bring some problems to your attention, please read the follow-up below.


I'm not aware of any patches needed for building on Windows.

The issue with the GCC attribute has already been fixed on master. However, assuming that I didn't miss any Windows-specific mandatory settings, master simply doesn't build - right out the gate. The config header template seems to be missing some variables here. There are other build errors in the
commit with which I'm trying to work (6eb844d ...).

Another issue is the distribution custom commands - one should not assume the existence of xz-utils on any platform. At the very least, one should use find_program and abort configuration if nothing is found - or make the whole thing optional. Better yet, simply use CPack for packaging. A TGZ generator is readily available with CMake on all platforms (IIRC) - as is a ZIP generator, which I'd prefer for Windows (even if the only reason is basic OS support for the archive type).

Regarding the distcheck target, unless you're using something like Cygwin (or for a subset of the utilized commands, the PowerShell), there is no way you're writing to /dev/null, calling rm or sh or any other Unix command on Windows. 😉


as long as they don't break stuff on Unix systems

It'd be useful to know the minimum-viable set of supported platforms and distros. I probably cannot assume that building on Ubuntu and Windows is enough, can I? I couldn't find anything about this in the docs tho.

Some examples currently cannot build on Windows (for instance, norandomopen uses unistd.h which isn't available with MSVC). I assume you want them and the regression tests to work cross-platform, correct?

I find that manpage generation on Windows isn't desirable. The real problem is that even HTML documentation generation is dependent on mandoc. A way to independently generate HTML or manpages could be doxygen, albeit this would introduce a new dependency (which shouldn't be a problem on any reasonable Linux distro or any of the BSDs) and would force one to rewrite documentation in the doxygen format.

@0-wiz-0
Copy link
Member

0-wiz-0 commented Feb 15, 2018

Thank you, you touch a lot of interesting points.

As for appveyor, we'd be very happy to have that working for libzip. I have tried setting it up a few months ago, but gave up after about a day of not getting it going correctly. You can still find my .appveyor* file in the git history, I removed it in f0b8dda. Please note that appveyor currently has different settings directly in its web configuration than were in that file.

Various people have reported success building on Windows, but I guess there are many different ways to build this on various different Windows systems, so please just send patches for what you need fixed.

The dist and distcheck targets are currently only intended to be run on Unix systems. If you want to make them work on Windows too, please go ahead, but I don't think this is high priority.

As for supported systems, we regularly build on travis (Linux, MacOS) and NetBSD, and the releases are also build-tested on Solaris/Illumos. If your changes keep it working on Linux/OSX/Windows that's good enough, we can fix the rest.

As for the test suite: some of our test use library pre-loading to override functions with test functions (especially the AES tests). I'm not sure if that'll work on Windows, and so far we didn't have the aim or the knowledge to get the tests working on Windows, but we'll gladly accept patches.

We're not planning on rewriting the documentation in a different format. The one for the latest release is always available as HTML on libzip.org. Is there a mandoc port for Windows?

Looking forward to your PRs!

@0-wiz-0
Copy link
Member

0-wiz-0 commented Feb 20, 2018

For some other needed Windows work, please see https://libzip.org/libzip-discuss/msg00837.html -- we've replaced the Crypto code by using external libraries, that needs a new Windows backend too.

@janisozaur
Copy link
Contributor

@thokra1 please see my PR for adding appveyor.yml: #32

@janisozaur
Copy link
Contributor

See some relevant discussion: microsoft/vcpkg#2973

@0-wiz-0
Copy link
Member

0-wiz-0 commented Apr 6, 2018

For finding libzip, use the standard CMake FindPkgConfig module (see e.g. https://cmake.org/cmake/help/v3.11/module/FindPkgConfig.html).
As for the other points, we'll gladly accept PRs, but will not work on this ourselves.

@0-wiz-0 0-wiz-0 closed this as completed Apr 6, 2018
@jakitliang
Copy link

For finding libzip, use the standard CMake FindPkgConfig module (see e.g. https://cmake.org/cmake/help/v3.11/module/FindPkgConfig.html). As for the other points, we'll gladly accept PRs, but will not work on this ourselves.

I dont think using FindPkgConfig is a good solution. Because libzip-config.cmake was written and can be find_package(libzip REQUIRED).

The is ridiculous that libzip can be find_package but can not being target_link_libraries.

But everything works in wrong way. Makes people feel libzip-config.cmake is a fake cmake file which did nothing but exists on there.

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

No branches or pull requests

4 participants