Allow building with cmake (to ease building on windows) #16

trondn · 2016-01-27T14:54:37Z

The patch series allows for building on windows with the following steps:

git clone git://github.com/trondn/snappy
mkdir build
cd build
cmake -G "NMake Makefiles" ..\snappy
nmake all test

(It should also build on unix systems by dropping the "-G NMake Makefiles" part)

Optiligence · 2016-01-27T15:56:17Z

Just my 2 cents when i see this:
https://cmake.org/cmake/help/v3.4/module/GenerateExportHeader.html
https://cmake.org/cmake/help/v3.4/module/CMakePackageConfigHelpers.html (omitting this disregards an integral part of the cmake ecosystem)
for the vs compile fix see #10

(This allows for easier builds on Windows)

Auto generated by CLion to cover cmake, c/c++ and autotools

trondn · 2016-01-27T17:48:02Z

I've never used the GenerateExportHeader or PackageConfig thing before :-) Updated the set to use that

Nulifier · 2016-02-05T17:18:27Z

Your config.h.in file doesn't work for MSVC 2015. You need to either check for _MSC_VER <= 1900 or increment it as it still doesn't have ssize_t.

jakirkham · 2016-04-19T02:26:08Z

Would be great if this works on VS 2008 also.

ccleve · 2016-08-09T02:51:18Z

I cloned today's version of google/snappy and applied this pull request to it. It built fine and the unit tests ran fine as well. I'm on Windows 10 with Visual Studio 14. There were a few compile warnings (attached).

If the warnings are not of concern, then please accept this PR. It'll make building projects that depend on snappy easier.
warnings.txt

rapoth · 2016-08-27T23:09:42Z

@trondn Thank you for submitting this PR! I can confirm this works on today's version of google/snappy with similar warnings with Visual Studio 14 2015 on Windows 10.

When I run the unit tests, I get a warning:

Running microbenchmarks.
WARNING: Compiled without optimization, will be slow.

Do you by any chance happen to know why?

vrabaud

Looks pretty good to me !

vrabaud · 2017-02-08T15:18:53Z

CMakeLists.txt

+
+# I haven't looked at what the soname for the previous
+# versions of snappy... let's just start with 1.1.3
+SET(SNAPPY_SONAME "1.1.3")


That variable is useless now that you use SNAPPY_MAJOR in SET_TARGET_PROPERTIES.

vrabaud · 2017-02-08T15:19:34Z

CMakeLists.txt

@@ -0,0 +1,155 @@
+PROJECT(Snappy C CXX)


The project is CXX only no ?

We export a C only API by wrapping the C++ code. See snappy-c.h.

vrabaud · 2017-02-08T15:19:55Z

CMakeLists.txt

+
+TEST_BIG_ENDIAN(WORDS_BIG_ENDIAN)
+IF (WORDS_BIG_ENDIAN)
+    MESSAGE(STATUS "Builing on big endian system")


typo: Building

vrabaud

That should probably be its own PR no ? The smaller the PR, the better.

vrabaud · 2017-02-08T15:42:07Z

The three commits should probably be independent PR, not to lose track of the awesomeness of the CMake commit :)

vrabaud · 2017-02-08T15:46:24Z

For the warning in VS, maybe you can add something like:

if(NOT CMAKE_BUILD_TYPE)
  set(CMAKE_BUILD_TYPE "Release" CACHE
    "Build type: Release, Debug or RelWithDebInfo" STRING FORCE
  )
endif()

alkis · 2017-02-08T16:56:10Z

CMakeLists.txt

+
+ADD_DEFINITIONS(-DHAVE_CONFIG_H)
+
+INCLUDE_DIRECTORIES(BEFORE ${Snappy_SOURCE_DIR})


Forgive my Cmake noobishness, but Cmake best practices says do not use include_directories but target_include_directories instead.

alkis · 2017-02-08T16:56:30Z

CMakeLists.txt

+    ADD_DEFINITIONS(-D_CRT_SECURE_NO_WARNINGS)
+ENDIF (WIN32)
+
+ADD_DEFINITIONS(-DHAVE_CONFIG_H)


target_add_definitions instead?

alkis · 2017-02-08T23:20:57Z

snappy-c.h

@@ -68,6 +69,7 @@ typedef enum {
 *   }
 *   free(output);
 */
+SNAPPY_PUBLIC_API


Is it possible to remove all SNAPPY_PUBLIC_API and use WINDOWS_EXPORT_ALL_SYMBOLS instead?

jakirkham · 2017-03-22T19:08:16Z

Not to be noisy, but is there any news on this PR?

Edit: I guess the work is continuing in PR ( #29 ).

alkis · 2017-06-02T07:24:36Z

Closing this because CMake support has been added internally and will be part of the next release.

trondn force-pushed the master branch from d5b1bf8 to 33cbe89 Compare January 27, 2016 17:46

trondn added 2 commits January 27, 2016 18:46

Add support for building with cmake

10dfec6

(This allows for easier builds on Windows)

Add .gitignore file

96278e7

Auto generated by CLion to cover cmake, c/c++ and autotools

trondn force-pushed the master branch from 33cbe89 to 14bcf73 Compare March 2, 2016 16:58

Compile fixes for Visual Studio 201[35] (using cmake & nmake)

782bafe

trondn force-pushed the master branch from 14bcf73 to 782bafe Compare March 2, 2016 16:59

jakirkham mentioned this pull request Apr 19, 2016

Add snappy conda-forge/staged-recipes#193

Closed

This was referenced Nov 7, 2016

Build for Windows conda-forge/blosc-feedstock#4

Closed

Add Windows support conda-forge/snappy-feedstock#4

Closed

vrabaud reviewed Feb 8, 2017

View reviewed changes

alkis reviewed Feb 8, 2017

View reviewed changes

vrabaud mentioned this pull request Feb 27, 2017

Allow building with cmake (to ease building on windows), version 2 #29

Closed

alkis closed this Jun 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow building with cmake (to ease building on windows) #16

Allow building with cmake (to ease building on windows) #16

trondn commented Jan 27, 2016

Optiligence commented Jan 27, 2016

trondn commented Jan 27, 2016

Nulifier commented Feb 5, 2016

jakirkham commented Apr 19, 2016

ccleve commented Aug 9, 2016

rapoth commented Aug 27, 2016

vrabaud left a comment

vrabaud Feb 8, 2017

vrabaud Feb 8, 2017

alkis Feb 8, 2017

vrabaud Feb 8, 2017

vrabaud left a comment

vrabaud commented Feb 8, 2017

vrabaud commented Feb 8, 2017

alkis Feb 8, 2017 •

edited

Loading

alkis Feb 8, 2017

alkis Feb 8, 2017

jakirkham commented Mar 22, 2017 •

edited

Loading

alkis commented Jun 2, 2017


		ADD_DEFINITIONS(-DHAVE_CONFIG_H)

		INCLUDE_DIRECTORIES(BEFORE ${Snappy_SOURCE_DIR})

Allow building with cmake (to ease building on windows) #16

Allow building with cmake (to ease building on windows) #16

Conversation

trondn commented Jan 27, 2016

Optiligence commented Jan 27, 2016

trondn commented Jan 27, 2016

Nulifier commented Feb 5, 2016

jakirkham commented Apr 19, 2016

ccleve commented Aug 9, 2016

rapoth commented Aug 27, 2016

vrabaud left a comment

Choose a reason for hiding this comment

vrabaud Feb 8, 2017

Choose a reason for hiding this comment

vrabaud Feb 8, 2017

Choose a reason for hiding this comment

alkis Feb 8, 2017

Choose a reason for hiding this comment

vrabaud Feb 8, 2017

Choose a reason for hiding this comment

vrabaud left a comment

Choose a reason for hiding this comment

vrabaud commented Feb 8, 2017

vrabaud commented Feb 8, 2017

alkis Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

alkis Feb 8, 2017

Choose a reason for hiding this comment

alkis Feb 8, 2017

Choose a reason for hiding this comment

jakirkham commented Mar 22, 2017 • edited Loading

alkis commented Jun 2, 2017

alkis Feb 8, 2017 •

edited

Loading

jakirkham commented Mar 22, 2017 •

edited

Loading