Re: Windows patches

[Thomas Klausner <tk%giga.or.at@localhost>]

On Fri, Feb 05, 2010 at 04:56:09PM +0100, Patrick Spendrin wrote:
> Thomas Klausner schrieb:
> > a) why change the INCLUDE_DIRECTORIES?
> > b) why do you move the ADD_SUBDIRECTORY lines?
> a) probably not needed
> b) because the include_directories(${ZLIB_INCLUDE_DIR}) only does take
> effect if the binaries are compiled below. Thus I got a missing include
> for zlib.h.

Ok.

> _WIN32 is a common define for both mingw and msvc compilers. Exports can
> be applied for both compilers on the windows platform.
> zip_EXPORTS is a define from cmake on *all* platforms which tells you
> that your library is currently built. The standard is
> libraryname_EXPORTS, you can redefine it to whatever you want. The point
> is that on windows you need use dllexport when the library gets build
> and dllimport when it is used (so above code couldn't have worked on
> Visual Studio).

Thanks for the explanation.

[PATH_SEPARATOR]
> > The zip spec explicitly says that the path separator inside zip files
> > is '/'.
> > 
> But for the outside? e.g. how do you care about filenames when reading
> an archive from C:\Users\Patrick\Downloads\test.zip ?
> I think that is what the patch is about, this might make it a bit more
> problematic as you have to split inner and outer path handling.

Ok, you're probably right that mkstemp needs changing. The files in
lib/ don't need any changes. The ones in regress might need changing,
though I don't think the tests currently use the functionality.

> > Why this one?
> because this is kind of non-standard. installation of headers normally
> go to install-prefix/include or install-prefix/include/libraryname. I
> can't really force you to do that, but I am currently maintaining builds
> of some libraries and wouldn't really like to have more special things
> than needed.

The problem with zipconf.h is that it is installation dependent -- you
can use the same zip.h on all platforms, but zipconf.h is a generated
file and will be different.

> It should only contain patches that can be applied right away, this
> means that libzip doesn't build with msvc yet (will look for the rest
> later).

These patches look good and didn't break anything for me, so I
committed them. Thank you!

>  INCLUDE_DIRECTORIES(${ZLIB_INCLUDE_DIR})
> +set(CMAKE_REQUIRED_INCLUDES ${ZLIB_INCLUDE_DIR})

Though I wonder why that really is necessary and the
INCLUDE_DIRECTORIES isn't sufficient. Oh well.

Cheers,
 Thomas