Re: [PATCH] Cmake-ify c-ares -- v2
On Tue, 18 Oct 2016, Gregor Jasny wrote: Gregor, you can merge this yourself, can't you? I cannot see / find the merge button. Ah sorry, that was my mistake. I intended for you to have that ability. Check again now! -- / daniel.haxx.se
Re: [PATCH] Cmake-ify c-ares -- v2
On 17/10/2016 10:29, Daniel Stenberg wrote: > On Sun, 16 Oct 2016, Gregor Jasny via c-ares wrote: > >> I think this branch is mergeable. Any of the requested changes can be >> made with some small follow-up commits. > > Gregor, you can merge this yourself, can't you? I cannot see / find the merge button.
Re: [PATCH] Cmake-ify c-ares -- v2
On 11/10/2016 15:31, Brad House via c-ares wrote: > Any chance this pull request will be merged before too long? I don't like > letting things just hang around. I think this branch is mergeable. Any of the requested changes can be made with some small follow-up commits. Thanks, Gregor
Re: [PATCH] Cmake-ify c-ares -- v2
On 10/03/2016 02:03 PM, Brad House via c-ares wrote: > On 10/03/2016 05:23 AM, Gregor Jasny wrote: >> On 29/09/2016 21:34, Gregor Jasny wrote: >>> On 29/09/2016 14:34, David Drysdale wrote: Gregor / Daniel, did you have any thoughts on the CMake change? >>> >>> Yes, I'm reviewing it but the CVE took most of my spare time. >> >> I think as soon as the latest comments are addressed, the PR is good to >> be merged. Any later changes could be done incrementally. >> >> I'm still no big fan of building shared and static libraries within the >> same build directory. But this defaults to "OFF" so it does not harm either. > > I'm pretty sure everything necessary has been addressed for merging, unless > anyone has any problems with it. > > -Brad > Any chance this pull request will be merged before too long? I don't like letting things just hang around. Thanks. -Brad
Re: [PATCH] Cmake-ify c-ares -- v2
On 10/03/2016 05:23 AM, Gregor Jasny wrote: > On 29/09/2016 21:34, Gregor Jasny wrote: >> On 29/09/2016 14:34, David Drysdale wrote: >>> >>> Gregor / Daniel, did you have any thoughts on the CMake change? >> >> Yes, I'm reviewing it but the CVE took most of my spare time. > > I think as soon as the latest comments are addressed, the PR is good to > be merged. Any later changes could be done incrementally. > > I'm still no big fan of building shared and static libraries within the > same build directory. But this defaults to "OFF" so it does not harm either. I'm pretty sure everything necessary has been addressed for merging, unless anyone has any problems with it. -Brad
Re: [PATCH] Cmake-ify c-ares -- v2
Hello, On 29/09/2016 21:34, Gregor Jasny wrote: > On 29/09/2016 14:34, David Drysdale wrote: >> >> Gregor / Daniel, did you have any thoughts on the CMake change? > > Yes, I'm reviewing it but the CVE took most of my spare time. I think as soon as the latest comments are addressed, the PR is good to be merged. Any later changes could be done incrementally. I'm still no big fan of building shared and static libraries within the same build directory. But this defaults to "OFF" so it does not harm either. Thanks, Gregor
Re: [PATCH] Cmake-ify c-ares -- v2
On 9/30/16 3:05 PM, Daniel Stenberg wrote: On Fri, 30 Sep 2016, Brad House via c-ares wrote: Anyone have thoughts on this? It would mean less to maintain for sure with multiple build systems, with the caveat of it being hard to fix if it doesn't work in the future for some reason. Makefile.inc is made like this exactly to allow all build systems to get the file names from a central place instead of having them in multiple places which always leads to files being out of sync every now and then. I vote for having the list of files in a single place. Merged into the pull request. -Brad
Re: [PATCH] Cmake-ify c-ares -- v2
On Fri, 30 Sep 2016, Brad House via c-ares wrote: Anyone have thoughts on this? It would mean less to maintain for sure with multiple build systems, with the caveat of it being hard to fix if it doesn't work in the future for some reason. Makefile.inc is made like this exactly to allow all build systems to get the file names from a central place instead of having them in multiple places which always leads to files being out of sync every now and then. I vote for having the list of files in a single place. -- / daniel.haxx.se
Re: [PATCH] Cmake-ify c-ares -- v2
Wow, impressive ... I think. Looks like voodoo to me :) Anyone have thoughts on this? It would mean less to maintain for sure with multiple build systems, with the caveat of it being hard to fix if it doesn't work in the future for some reason. -Brad On 09/30/2016 02:24 PM, bdoetsch wrote: > Hey Brad, > > I found a cmake function that converts Makefile.inc files into cmake > directives, already in use sitting in the curl project (this one is from > curl-7.43.0). It allows the CMake build to use all the ".c" and ".h" files > already defined in Makefile.inc using a bunch of regex replacements. Not > sure how > resilient it is, but it's working for me on your latest github commit > (e46e59454c9e813822052d73e6c77efc731923e8) with macOS 10.11.6. Attached is a > patch against your CMakeLists.txt if you're interested in it. > > Thought maybe it could help. No worries either way, > > Brady > > > > > On 9/29/16 6:14 AM, Brad House via c-ares wrote: >> On 9/28/16 9:19 AM, Brad House via c-ares wrote: >>> On 9/28/16 7:40 AM, David Drysdale wrote: Any pull request will get run through Travis automatically [1], so you shouldn't need a Travis account. If you're not used to Travis, it's probably easier for me to add something -- first attempt at [2], with output at [3]. Does that look sensible? D. [1] https://travis-ci.org/c-ares/c-ares/builds/163269776 [2] https://github.com/daviddrysdale/c-ares/commit/fc7917e3c5b99ca4f9be66ea5060a2b49a5bbcec [3] https://travis-ci.org/daviddrysdale/c-ares/builds/163350713 >>> >>> Seems reasonable to me. Built both shared and static variants and test >>> utilities, >>> and tests ran successfully. >> >> >> David, should I import your travis modification commit into my repo so it >> is part of the c-ares pull request (64)? >> >> Thanks. >> -Brad > diff --git a/CMakeLists.txt b/CMakeLists.txt index f6a779d..7ef257d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -480,61 +480,96 @@ INCLUDE_DIRECTORIES ( ${CARES_INCLUDE_DIRS} ) +# TRANSFORM_MAKEFILE_INC +# +# This function consumes the "Makefile.inc" autotools file, and converts it into +# "Makefile.inc.cmake", a cmake include file; transforming this: +# +# CSOURCES = ares__close_sockets.c \ +# ares__get_hostent.c\ +# ares__read_line.c \ +# ... +# +# into this: +# +# SET (CSOURCES +# ares__close_sockets.c +# ares__get_hostent.c +# ares__read_line.c +# ... +function(TRANSFORM_MAKEFILE_INC INPUT_FILE OUTPUT_FILE) + file(READ ${INPUT_FILE} MAKEFILE_INC_TEXT) + string(REPLACE "$(top_srcdir)" "\${CURL_SOURCE_DIR}" MAKEFILE_INC_TEXT ${MAKEFILE_INC_TEXT}) + string(REPLACE "$(top_builddir)" "\${CURL_BINARY_DIR}" MAKEFILE_INC_TEXT ${MAKEFILE_INC_TEXT}) + + string(REGEX REPLACE "\n" "ß!ß" MAKEFILE_INC_TEXT ${MAKEFILE_INC_TEXT}) + string(REGEX REPLACE "([a-zA-Z_][a-zA-Z0-9_]*)[\t ]*=[\t ]*([^\n]*)" "SET(\\1 \\2)" MAKEFILE_INC_TEXT ${MAKEFILE_INC_TEXT}) + string(REPLACE "ß!ß" "\n" MAKEFILE_INC_TEXT ${MAKEFILE_INC_TEXT}) + + string(REGEX REPLACE "\\$\\(([a-zA-Z_][a-zA-Z0-9_]*)\\)" "\${\\1}" MAKEFILE_INC_TEXT ${MAKEFILE_INC_TEXT})# Replace $() with ${} + string(REGEX REPLACE "@([a-zA-Z_][a-zA-Z0-9_]*)@" "\${\\1}" MAKEFILE_INC_TEXT ${MAKEFILE_INC_TEXT})# Replace @@ with ${}, even if that may not be read by CMake scripts. + file(WRITE ${OUTPUT_FILE} ${MAKEFILE_INC_TEXT}) +endfunction() + +# run the function... +transform_makefile_inc("Makefile.inc" "${PROJECT_BINARY_DIR}/Makefile.inc.cmake") +include(${PROJECT_BINARY_DIR}/Makefile.inc.cmake) + # Source files. -SET (CARES_SOURCES - ares__close_sockets.c - ares__get_hostent.c - ares__read_line.c - ares__timeval.c - ares_cancel.c - ares_data.c - ares_destroy.c - ares_expand_name.c - ares_expand_string.c - ares_fds.c - ares_free_hostent.c - ares_free_string.c - ares_getenv.c - ares_gethostbyaddr.c - ares_gethostbyname.c - ares_getnameinfo.c - ares_getsock.c - ares_init.c - ares_library_init.c - ares_llist.c - ares_mkquery.c - ares_create_query.c - ares_nowarn.c - ares_options.c - ares_parse_a_reply.c - ares_parse__reply.c - ares_parse_mx_reply.c - ares_parse_naptr_reply.c - ares_parse_ns_reply.c - ares_parse_ptr_reply.c - ares_parse_soa_reply.c - ares_parse_srv_reply.c - ares_parse_txt_reply.c - ares_platform.c - ares_process.c - ares_query.c - ares_search.c - ares_send.c - ares_strcasecmp.c - ares_strdup.c - ares_strerror.c - ares_timeout.c - ares_version.c - ares_writev.c - bitncmp.c - inet_net_pton.c - inet_ntop.c - windows_port.c -) +# SET (CSOURCES +# ares__close_sockets.c +#
Re: [PATCH] Cmake-ify c-ares -- v2
On Thu, Sep 29, 2016 at 12:14 PM, Brad Housewrote: > On 9/28/16 9:19 AM, Brad House via c-ares wrote: >> >> On 9/28/16 7:40 AM, David Drysdale wrote: >>> >>> Any pull request will get run through Travis automatically [1], so you >>> shouldn't need a Travis account. >>> >>> If you're not used to Travis, it's probably easier for me to add >>> something -- first attempt at [2], with output at [3]. Does that look >>> sensible? >>> >>> D. >>> >>> [1] https://travis-ci.org/c-ares/c-ares/builds/163269776 >>> [2] >>> https://github.com/daviddrysdale/c-ares/commit/fc7917e3c5b99ca4f9be66ea5060a2b49a5bbcec >>> [3] https://travis-ci.org/daviddrysdale/c-ares/builds/163350713 >>> >> >> Seems reasonable to me. Built both shared and static variants and test >> utilities, >> and tests ran successfully. > > > > David, should I import your travis modification commit into my repo so it > is part of the c-ares pull request (64)? > > Thanks. > -Brad Either that, or I'll just add it to the repo when/if the cmake stuff goes in...
Re: [PATCH] Cmake-ify c-ares -- v2
On Wed, Sep 28, 2016 at 1:10 AM, Brad House via c-areswrote: > On 9/27/16 9:52 AM, Gregor Jasny via c-ares wrote: >> >> Hi Brad, >> >> On 27/09/2016 13:44, Brad House via c-ares wrote: >>> >>> I've attached v2 of my CMake patch for c-ares. The changes are: >>> * Sync with master (no configure-time type size checks) >>> * Support iOS multi-arch building >>> * Require only CMake v2.8 >>> > ... >> >> >> If you don't mind could you please create a pull request on github and >> ping me (gjasny) there? >> >> Thanks, >> Gregor >> > > > done! > https://github.com/c-ares/c-ares/pull/64 > > -Brad Gregor / Daniel, did you have any thoughts on the CMake change?
Re: [PATCH] Cmake-ify c-ares -- v2
On 9/28/16 7:40 AM, David Drysdale wrote: On Wed, Sep 28, 2016 at 1:09 AM, Brad Housewrote: On 9/27/16 10:26 AM, David Drysdale via c-ares wrote: On Tue, Sep 27, 2016 at 2:52 PM, Gregor Jasny via c-ares wrote: Hi Brad, On 27/09/2016 13:44, Brad House via c-ares wrote: I've attached v2 of my CMake patch for c-ares. The changes are: * Sync with master (no configure-time type size checks) * Support iOS multi-arch building * Require only CMake v2.8 ... If you don't mind could you please create a pull request on github and ping me (gjasny) there? Thanks, Gregor Also, it would be great if you could add some integration with .travis.yml, so we can see the build results in the pull request. Thanks, David I must say, I've never messed with travis. Will I need to create a new travis account to test this, or is this something that if I modify the .travis.yml in the pull request it will cause it to build? Any pull request will get run through Travis automatically [1], so you shouldn't need a Travis account. If you're not used to Travis, it's probably easier for me to add something -- first attempt at [2], with output at [3]. Does that look sensible? D. [1] https://travis-ci.org/c-ares/c-ares/builds/163269776 [2] https://github.com/daviddrysdale/c-ares/commit/fc7917e3c5b99ca4f9be66ea5060a2b49a5bbcec [3] https://travis-ci.org/daviddrysdale/c-ares/builds/163350713 Seems reasonable to me. Built both shared and static variants and test utilities, and tests ran successfully. -Brad
Re: [PATCH] Cmake-ify c-ares -- v2
On Wed, Sep 28, 2016 at 1:09 AM, Brad Housewrote: > On 9/27/16 10:26 AM, David Drysdale via c-ares wrote: >> >> On Tue, Sep 27, 2016 at 2:52 PM, Gregor Jasny via c-ares >> wrote: >>> >>> Hi Brad, >>> >>> On 27/09/2016 13:44, Brad House via c-ares wrote: I've attached v2 of my CMake patch for c-ares. The changes are: * Sync with master (no configure-time type size checks) * Support iOS multi-arch building * Require only CMake v2.8 > ... >>> >>> >>> If you don't mind could you please create a pull request on github and >>> ping me (gjasny) there? >>> >>> Thanks, >>> Gregor >> >> >> Also, it would be great if you could add some integration with >> .travis.yml, >> so we can see the build results in the pull request. >> >> Thanks, >> David >> > > I must say, I've never messed with travis. Will I need to create a new > travis > account to test this, or is this something that if I modify the .travis.yml > in > the pull request it will cause it to build? Any pull request will get run through Travis automatically [1], so you shouldn't need a Travis account. If you're not used to Travis, it's probably easier for me to add something -- first attempt at [2], with output at [3]. Does that look sensible? D. [1] https://travis-ci.org/c-ares/c-ares/builds/163269776 [2] https://github.com/daviddrysdale/c-ares/commit/fc7917e3c5b99ca4f9be66ea5060a2b49a5bbcec [3] https://travis-ci.org/daviddrysdale/c-ares/builds/163350713
Re: [PATCH] Cmake-ify c-ares -- v2
On 9/27/16 9:52 AM, Gregor Jasny via c-ares wrote: Hi Brad, On 27/09/2016 13:44, Brad House via c-ares wrote: I've attached v2 of my CMake patch for c-ares. The changes are: * Sync with master (no configure-time type size checks) * Support iOS multi-arch building * Require only CMake v2.8 ... If you don't mind could you please create a pull request on github and ping me (gjasny) there? Thanks, Gregor done! https://github.com/c-ares/c-ares/pull/64 -Brad
Re: [PATCH] Cmake-ify c-ares -- v2
On 9/27/16 10:26 AM, David Drysdale via c-ares wrote: On Tue, Sep 27, 2016 at 2:52 PM, Gregor Jasny via c-areswrote: Hi Brad, On 27/09/2016 13:44, Brad House via c-ares wrote: I've attached v2 of my CMake patch for c-ares. The changes are: * Sync with master (no configure-time type size checks) * Support iOS multi-arch building * Require only CMake v2.8 ... If you don't mind could you please create a pull request on github and ping me (gjasny) there? Thanks, Gregor Also, it would be great if you could add some integration with .travis.yml, so we can see the build results in the pull request. Thanks, David I must say, I've never messed with travis. Will I need to create a new travis account to test this, or is this something that if I modify the .travis.yml in the pull request it will cause it to build? Thanks! -Brad
Re: [PATCH] Cmake-ify c-ares -- v2
Just following up on a couple of points from last time round, plus a question for Daniel below. On Tue, Sep 27, 2016 at 12:44 PM, Brad House via c-areswrote: > I've attached v2 of my CMake patch for c-ares. The changes are: [snip] - The CMake build seems to assume use of the main source code directory, but I've seen other CMake builds that allow parallel build subdirectories. If the latter is possible, it might allow an autoconf build and a CMake build to use a shared source tree, reducing the size of the Travis build matrix. >>> >>> >>> >>> Actually, that's not true. I never build using cmake in the main source >>> tree. I typically do something like: >>> >>> cd c-ares >>> mkdir build >>> cd build >>> cmake -DCMAKE_BUILD_TYPE=DEBUG -DCARES_STATIC=ON -DCARES_STATIC_PIC=ON .. >>> make This works OK for me now; not sure what I was doing wrong last time. >>> >>> However, I haven't seen what would happen if the autoconf version ran >>> in the source tree and CMake ran outside. I can't guarantee it would >>> reference the right ares_config.h/ares_build.h files. It *should* since >>> it sets the include path of the build directory first. >>> >>> - For tidiness it would be nice to update .gitignore to list any CMake build detritus. >>> >>> >>> >>> We could, but I typically wouldn't recommend building in the source >>> directory anyhow Its not like autoconf that has to run ./buildconf >>> to generate a bunch of files before ./configure can run so I'm not >>> sure if it makes sense to add any exclusions. Yep, makes sense. >>> A couple of other questions (from a CMake n00b): - Where does SOVERSION of 3.0.1 come from? >>> >>> I extracted that from the autotools Makefile.am: >>> CARES_VERSION_INFO = -version-info 3:0:1 >>> >>> I'm pretty sure these are equivalent. Daniel, what's the guidelines/process for updating CARES_VERSION_INFO for a release? It looks like it was updated for 1.10.0 but not for 1.11.0...
Re: [PATCH] Cmake-ify c-ares -- v2
I've attached v2 of my CMake patch for c-ares. The changes are: * Sync with master (no configure-time type size checks) * Support iOS multi-arch building * Require only CMake v2.8 The main thing that still needs to be done is to build the tests, however I'm not seeing how to do that with CMake 2.8 as was requested since it doesn't have C++11 support (needed to set the right flags during compilation). Even with later CMake versions, I'm not exactly sure how to detect a C++11 compiler ... so it will take quite a bit of research. (It doesn't help that I'm a C guy and try not to touch C++). That said, I don't know if that is a show-stopper for an experimental build system distributed in parallel with the primary autotools build system. -Brad On 6/28/16 8:42 AM, Brad House wrote: I've just confirmed CMake 2.8 (tested 2.8.12.2 on Ubuntu 14.04) works fine, it doesn't appear I'm depending on any v3 features, so it is safe to reduce the minimum version. -Brad On 6/28/16 7:56 AM, Brad House via c-ares wrote: Commenting below ... On 6/28/16 6:42 AM, David Drysdale via c-ares wrote: Hi Brad, This seems to run pretty well -- as you say, it's vastly faster than the ./configure variant! :) My main concern would be that having 2 parallel build systems adds a bit more friction to the development process (e.g. anyone adding a new source code file needs to remember to update/test both), and we don't necessarily have any folk knowledgeable about CMake. However, given that c-ares is pretty stable, that might not be a big issue, particularly if we document that the autotools variant is the "official" build system. I agree, I couldn't figure out any way to make use of the Makefile.inc which lists the source files, which would have been convenient. Since C-Ares releases only happen once every couple of years, I can definitely make sure I do any regression testing across the platforms I have access to prior to release to look for anything that might have broken. Technically there is another build system too, the Windows "NMake" files, but autotools and windows nmake are able to share the Makefile.inc for the list of sources. One thing to help would be to set up automatic builds -- if we set up Travis / AppVeyor build variants for CMake, we should be able to quickly spot any omissions. Obviously, including the tests in the CMake setup would ideally be needed, and there's a few other things that might need tweaking: Hooking into Travis sounds like a good idea. I need to wrap my head around the test system to figure out how to integrate that still. - Your patch requires CMake 3.0, which is later than is installed at Travis (which has 2.8.7 I think). Do you need 3.x features, or could you set a lower minimum version? I'll need to test to see if there was really any reason to require CMake v3 or not. I know we found a reason in some other projects, but I think it was behavior of FindPackage(OpenSSL) that was broken until v3... obviously not relevant here. - The CMake build seems to assume use of the main source code directory, but I've seen other CMake builds that allow parallel build subdirectories. If the latter is possible, it might allow an autoconf build and a CMake build to use a shared source tree, reducing the size of the Travis build matrix. Actually, that's not true. I never build using cmake in the main source tree. I typically do something like: cd c-ares mkdir build cd build cmake -DCMAKE_BUILD_TYPE=DEBUG -DCARES_STATIC=ON -DCARES_STATIC_PIC=ON .. make ... However, I haven't seen what would happen if the autoconf version ran in the source tree and CMake ran outside. I can't guarantee it would reference the right ares_config.h/ares_build.h files. It *should* since it sets the include path of the build directory first. - For tidiness it would be nice to update .gitignore to list any CMake build detritus. We could, but I typically wouldn't recommend building in the source directory anyhow Its not like autoconf that has to run ./buildconf to generate a bunch of files before ./configure can run so I'm not sure if it makes sense to add any exclusions. A couple of other questions (from a CMake n00b): - Where does SOVERSION of 3.0.1 come from? I extracted that from the autotools Makefile.am: CARES_VERSION_INFO = -version-info 3:0:1 I'm pretty sure these are equivalent. - Is there any way to automatically get VERSION 1.11.0 from ares_version.h so there's fewer things to manually sync? I think the actual C-Ares VERSION can probably be removed completely from the line you are referring to. I don't believe it is actually being used for anything, the SOVERSION is the important one. I don't think we can easily extract VERSION from ares_version.h because we don't have access to a posix shell like we could assume with autotools. We might be able to build an executable just for that using the ares_version.h, but I'm not sure how that might impact