Integrate booststrap-dht with cmake#5
Integrate booststrap-dht with cmake#5davidchappelle wants to merge 3 commits intobittorrent:masterfrom
Conversation
|
(sorry for taking so long to respond). Is there a compelling reason to add a dependency on libtorrent just for that one file? |
|
Can you be more specific? I am not seeing what your comment is applying to. |
CMakeLists.txt
Outdated
There was a problem hiding this comment.
this adds a dependency on libtorrent, doesn't it?
There was a problem hiding this comment.
Ah yes I see. I'll fix that up.
On Fri, Jul 10, 2015 at 5:47 PM, Arvid Norberg notifications@github.com
wrote:
In CMakeLists.txt
#5 (comment):@@ -0,0 +1,27 @@
+cmake_minimum_required(VERSION 2.8)
+project(bootstrap-dht)
+
+set(CMAKE_MODULE_PATH
- ${CMAKE_MODULE_PATH}
- ${CMAKE_CURRENT_SOURCE_DIR}/cmake/Modules/)
+set(CMAKE_CXX_FLAGS "-std=c++11 -Wall -Werror ${CMAKE_CXX_FLAGS}")
+
+find_package(LibTorrentRasterbar REQUIRED)this adds a dependency on libtorrent, doesn't it?
—
Reply to this email directly or view it on GitHub
https://github.com/bittorrent/bootstrap-dht/pull/5/files#r34399336.
There was a problem hiding this comment.
Ok I pushed a fix.
On Fri, Jul 10, 2015 at 6:33 PM, David Chappelle chappedm@gmail.com wrote:
Ah yes I see. I'll fix that up.
On Fri, Jul 10, 2015 at 5:47 PM, Arvid Norberg notifications@github.com
wrote:In CMakeLists.txt
#5 (comment)
:@@ -0,0 +1,27 @@
+cmake_minimum_required(VERSION 2.8)
+project(bootstrap-dht)
+
+set(CMAKE_MODULE_PATH
- ${CMAKE_MODULE_PATH}
- ${CMAKE_CURRENT_SOURCE_DIR}/cmake/Modules/)
+set(CMAKE_CXX_FLAGS "-std=c++11 -Wall -Werror ${CMAKE_CXX_FLAGS}")
+
+find_package(LibTorrentRasterbar REQUIRED)this adds a dependency on libtorrent, doesn't it?
—
Reply to this email directly or view it on GitHub
https://github.com/bittorrent/bootstrap-dht/pull/5/files#r34399336.
There was a problem hiding this comment.
this whole file still has lots of references to libtorrent. There is no dependency on libtorrent, right?
Oh, I just realized this is the libtorrent cmake module. it seems odd that this file would be necessary.
Although Jam may be the preferred build tool in many cases, cmake is also a very commonly used build tool. Adding cmake support simplifies integration of bootstrap-dht for those environments.
The common pattern with cmake from the top level directory is:
mkdir build
cd build
cmake ../
This is typically referred to as an out of source build. This leaves
your project tree intact and does not pollute it with build output
files. As a result, it makes sense to exclude this directory so that
it doesn't accidentally get checked in to git.
|
Thanks for the feedback Arvid. I think I have addressed all of the review comments now. |
2 participants
This will make integration for those using cmake much easier. I also fixed up a bunch of compiler warnings about singed/unsigned comparisons.