Allow building with cmake (to ease building on windows), version 2 by vrabaud · Pull Request #29 · google/snappy

vrabaud · 2017-02-27T13:24:35Z

This is a small rewrite of #16 , mostly to simplify the windows support and modify less code.

googlebot · 2017-02-27T13:24:39Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

vrabaud · 2017-02-28T08:25:00Z

@trondn, I can take over this pull request until it's merged but I would need you to agree to the Contributor License Agreeement. Is that ok with you ?

trondn · 2017-02-28T08:30:44Z

Sure :) (I don't know why the CLA check failed.. I've singed the CLA earlier..)

googlebot · 2017-02-28T08:38:36Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

vrabaud · 2017-02-28T08:40:33Z

ok, @trondn, the error was on my side, sorry. As asked by the bot, please confirm that you are ok with me committing what you authored, and we are good to go ! Thx !

trondn · 2017-02-28T08:44:15Z

Go for it :)

alkis · 2017-02-28T15:12:38Z

+CHECK_LIBRARY_EXISTS(fastlz fastlz_compress "" HAVE_LIBFASTLZ)
+CHECK_LIBRARY_EXISTS(quicklz qlz_compress "" HAVE_LIBQUICKLZ)
+
+CHECK_CXX_SOURCE_COMPILES("int main(void) { return __builtin_expect(0, 1) ? 1 : 0; }"


Wouldn't the following be enough:
CHECK_CXX_SOURCE_COMPILES("int main(void) { return __builtin_expect(0, 1); }" HAVE_BUILTIN_EXPECT)

alkis · 2017-02-28T15:12:51Z

+CHECK_CXX_SOURCE_COMPILES("int main(void) { return __builtin_expect(0, 1) ? 1 : 0; }"
+ HAVE_BUILTIN_EXPECT)
+
+CHECK_CXX_SOURCE_COMPILES("int main(void) { return __builtin_ctzll(0) ? 1 : 0; }"


alkis · 2017-02-28T15:16:50Z

+
+SET(SNAPPY_MAJOR 1)
+SET(SNAPPY_MINOR 1)
+SET(SNAPPY_PATCHLEVEL 3)


This should be PATCHLEVEL 4.

Optiligence · 2017-03-22T20:17:38Z

you can specify the version in the project command instead of manually setting it
SOVERSION will then be set automatically and WRITE_BASIC_PACKAGE_VERSION_FILE will also use it automatically.

Optiligence · 2017-03-22T22:10:00Z

everything ${CMAKE_INSTALL_PREFIX}/lib/cmake should be lib/cmake/Snappy, it currently installs to <build_dir>/<install_prefix>/<install_prefix>/lib/Snappy/cmake if a relative CMAKE_INSTALL_PREFIX is used

…n windows), version 2

Optiligence · 2017-03-22T23:39:59Z

also, the export set is currently kinda useless because it doesn’t export any targets (and this is basically what #16 (comment) was intended to be about)

i’d also vote for using lower case for the CMake code

I added proposed changes here https://github.com/Optiligence/snappy/tree/cmake
I’m not sure how to proceed with this though.

(This allows for easier builds on Windows)

vrabaud · 2017-03-23T13:12:19Z

@Optiligence , I believe I took your comments into account in a new commit (except fro capitalization for now). I also added some older fixes that I had squashed in the older CMakeLists.txt commit (shouldn't have sorry).

Optiligence · 2017-03-23T14:18:20Z

Several parts are different (missing, wrong), you can squash the commits from here https://github.com/Optiligence/snappy/tree/cmake_upper_case instead
although i’d really do the capitalization now too as a baseline instead of sometime later.
(I can also change the capitalization of my changes to upper case, although that defeats my intention of having it all lower case in the end.)
I also added a line on my previous HEAD commit and added another commit regarding static and shared flavors.

Optiligence · 2017-03-23T14:25:29Z

I was also thinking that the output name should contain the ABI version, as it currently does with autotools.

KindDragon · 2017-04-03T16:44:25Z

Duplicate from line 24

We need CMake 3.4 which is not in 14.04 hence the jump to 16.04.

wesm · 2017-04-18T19:23:16Z

Any updates here / anything others can do to help? We would love to put up a Snappy windows build on https://github.com/conda-forge/snappy-feedstock

alkis · 2017-04-22T09:15:02Z

Yes we are making progress. We should have arelease with cmake soon, plus continuous integration. On Apr 18, 2017 21:23, "Wes McKinney" <notifications@github.com> wrote: Any updates here / anything others can do to help? We would love to put up a Snappy windows build on https://github.com/conda-forge/snappy-feedstock — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#29 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAQO_bq7jxNyZ048ecAAdm9Ch1nQKA8Uks5rxQ2lgaJpZM4MNGTE> .

KindDragon · 2017-05-05T07:51:40Z

Please add CMake option to build tests https://gist.github.com/KindDragon/b8752d22541a6d9d488e066b6a93a853#file-snappycmakelists-txt-L113

alkis · 2017-06-02T06:55:57Z

I am closing this PR since we have submitted CMake internally and it will be part of the next release.

This lands google#29

vrabaud closed this Feb 28, 2017

vrabaud reopened this Feb 28, 2017

vrabaud force-pushed the cmake branch from e47ca7a to d9555c0Compare February 28, 2017 08:39

alkis suggested changes Feb 28, 2017
View reviewed changes

jakirkham mentioned this pull request Mar 22, 2017
Allow building with cmake (to ease building on windows) #16
Closed

Optiligence pushed a commit to Optiligence/snappy that referenced this pull request Mar 22, 2017
Pull Requestgoogle#29: Allow building with cmake (to ease building o…
4aef60d
…n windows), version 2

vrabaud force-pushed the cmake branch from d9555c0 to f810875Compare March 23, 2017 08:32

trondn added 2 commits March 23, 2017 09:41

Add support for building with cmake
fddfc36
(This allows for easier builds on Windows)

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

vrabaud force-pushed the cmake branch 5 times, most recently from ae6f3d6 to 5188792Compare March 23, 2017 13:08

vrabaud force-pushed the cmake branch from 5188792 to cb53a06Compare March 23, 2017 13:21

vrabaud force-pushed the cmake branch 3 times, most recently from 523dc27 to cf1aad0Compare March 24, 2017 08:55

vrabaud force-pushed the cmake branch 2 times, most recently from 8145ab6 to 9b8e53aCompare March 24, 2017 10:58

KindDragon reviewed Apr 3, 2017
View reviewed changes

Comment threadCMakeLists.txt Outdated
KindDragonApr 3, 2017
Copy link
Copy Markdown
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate from line 24

vrabaud force-pushed the cmake branch from 9b8e53a to 1c5d497Compare April 6, 2017 08:50

vrabaud added 2 commits April 6, 2017 16:15

add more CMake fixes.
a433e00

Set CMake and Ubuntu 16.04 as default on Travis.
3a4cfc6
We need CMake 3.4 which is not in 14.04 hence the jump to 16.04.

vrabaud force-pushed the cmake branch from 1c5d497 to 3a4cfc6Compare April 6, 2017 14:15

alkis closed this Jun 2, 2017

pwnall pushed a commit to pwnall/snappy that referenced this pull request Jun 5, 2017
Provide a CMakeLists.txt.
c813168
This lands google#29

Optiligence mentioned this pull request Jul 20, 2017
CMake improvements #38
Closed

Uh oh!

Conversation

vrabaud commented Feb 27, 2017

Uh oh!

googlebot commented Feb 27, 2017

Uh oh!

vrabaud commented Feb 28, 2017

Uh oh!

trondn commented Feb 28, 2017

Uh oh!

googlebot commented Feb 28, 2017

Uh oh!

vrabaud commented Feb 28, 2017

Uh oh!

trondn commented Feb 28, 2017

Uh oh!

alkisFeb 28, 2017

Choose a reason for hiding this comment

Uh oh!

alkisFeb 28, 2017

Choose a reason for hiding this comment

Uh oh!

alkisFeb 28, 2017

Choose a reason for hiding this comment

Uh oh!

Optiligence commented Mar 22, 2017

Uh oh!

Optiligence commented Mar 22, 2017

Uh oh!

Optiligence commented Mar 22, 2017

Uh oh!

vrabaud commented Mar 23, 2017

Uh oh!

Optiligence commented Mar 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Optiligence commented Mar 23, 2017

Uh oh!

KindDragonApr 3, 2017

Choose a reason for hiding this comment

Uh oh!

wesm commented Apr 18, 2017

Uh oh!

alkis commented Apr 22, 2017 via email

Uh oh!

KindDragon commented May 5, 2017

Uh oh!

alkis commented Jun 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Optiligence commented Mar 23, 2017•
edited
Loading