build: add a cmake build file #1850
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cjihrig
approved these changes
May 28, 2018
Contributor
cjihrig
left a comment
cjihrig
left a comment
There was a problem hiding this comment.
Thank you for breaking this up into small commits :-D
LGTM as best I can tell.
Closed
|
Looks great! I have a few nitpicks.
|
Contributor
|
I'm on Windows; installing libraries isn't really a thing I expect to do. Is there a way to configure without? |
Contributor
|
With this simple patch applied, it passes the configuration stage: diff --git a/CMakeLists.txt b/CMakeLists.txt
index bd101ad4..4bfd7574 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -360,8 +360,10 @@ set(libdir ${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR})
set(prefix ${CMAKE_INSTALL_PREFIX})
configure_file(libuv.pc.in ${CMAKE_CURRENT_BINARY_DIR}/libuv.pc @ONLY)
-install(DIRECTORY include/ DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
-install(FILES LICENSE ${CMAKE_CURRENT_BINARY_DIR}/libuv.pc
- DESTINATION ${CMAKE_INSTALL_DOCDIR})
-install(TARGETS uv LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR})
-install(TARGETS uv_a ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})
+if (DESTINATION)
+ install(DIRECTORY include/ DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
+ install(FILES LICENSE ${CMAKE_CURRENT_BINARY_DIR}/libuv.pc
+ DESTINATION ${CMAKE_INSTALL_DOCDIR})
+ install(TARGETS uv LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR})
+ install(TARGETS uv_a ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})
+endif(DESTINATION) |
Contributor
|
It's a little strange that the generated project has both a |
Contributor
|
As far as building is concerned - it bows out on not being able to find Which can be solved with: diff --git a/src/win/timer.c b/src/win/timer.c
index 7e006fed..eda5c24f 100644
--- a/src/win/timer.c
+++ b/src/win/timer.c
@@ -24,7 +24,7 @@
#include "uv.h"
#include "internal.h"
-#include "tree.h"
+#include "uv/tree.h"
#include "handle-inl.h" |
bnoordhuis
force-pushed
the
cmake
branch
2 times, most recently
from
144fa68 to
a24b737
Compare
Member
Author
|
Suggestions and fixes included. CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/889/
Apropos |
bnoordhuis
commented
May 30, 2018
| @@ -0,0 +1,381 @@ | |||
| # TODO: determine CMAKE_SYSTEM_NAME on OS/390. Currently assumes "OS/390". | |||
Contributor
Configuration works! Building the tests fails now: |
| @@ -0,0 +1,381 @@ | |||
| # TODO: determine CMAKE_SYSTEM_NAME on OS/390. Currently assumes "OS/390". | |||
| cmake_minimum_required(VERSION 3.0) | |||
| project(libuv) | |||
Contributor
There was a problem hiding this comment.
Maybe change the project name to be "uv" rather than "libuv". This is more in line with what the gyp build does.
Member
Author
There was a problem hiding this comment.
I surveyed what other cmake-based projects call their libraries and 'libfoo' - not 'foo' - seems to be the standard.
Contributor
|
I'm testing on Windows with the ninja generator ( With testing on I'm getting similar results: IMHO targeting ninja an "official" builder will also be beneficial (simpler, faster, more predictable, less disparity with *nix, easier to integrate |
Contributor
|
Suggested fixes: piscisaureus/libuv@cmake-win~2...piscisaureus:cmake-win |
Contributor
|
@piscisaureus's patch works with ninja as well (BTW I folded your build output) |
Contributor
Cool, didn't know you could do that. |
Contributor
|
I would like to suggest the following patch: bnoordhuis/libuv@cmake...mikefero:cmake I wanted to address @piscisaureus concerns where I also corrected a build issue with older MSVC compilers and tested Visual Studio 2010 - Visual Studio 2017 (32 and 64-bit builds) and added benchmark executables to the mix as well. I understand that with libuv v2.0.0 you will be dropping support from these older compilers, but for now building against the older compilers is very handy as Visual Studio 2010/2012/2013 are not ESOL just yet (some people just can't get with the times). |
Contributor
|
To aid in evaluating the patch suggestions I updated the AppVeyor CI configuration (separate branch: mikefero/libuv@cmake...mikefero:cmake_appveyor) and executed builds against Visual Studio 2010 - Visual Studio 2017 (32 and 64-bit builds) along with GYP builds (with static library build added to the matrix). The CI run can be found here: https://ci.appveyor.com/project/mikefero/libuv/build/v1.18.0.build1. NOTE: I also ran local builds on macOS High Sierra (10.13.4), Ubuntu 16.04, and Ubuntu 18.04 to ensure that unix-like environments were still valid with the patch suggestions. |
This is a cherry-pick of commit d010030 from the master branch. Conflicts: Makefile.am include/uv.h include/uv/unix.h libuv.nsi (deleted) src/unix/pthread-barrier.c (deleted) PR-URL: libuv#1850 Reviewed-By: Colin Ihrig <[email protected]>
Move it so that include/ contains uv.h and nothing more. PR-URL: libuv#1850 Reviewed-By: Colin Ihrig <[email protected]>
Use the right file path and variable name for the posix.h header file. Introduced when commit ce41af2 ("cygwin: include uv-posix.h header") was merged from the v1.x branch, where it is the correct path, into the master branch. This is a cherry-pick of commit d0c2ad3 from the master branch. PR-URL: libuv#1850 Reviewed-By: Colin Ihrig <[email protected]>
MSYS2 + MinGW-w64 have good support for autoconf, that should be the preferred way of building libuv under MinGW. This is a cherry-pick of commit ee949df from the master branch. PR-URL: libuv#1850 Reviewed-By: Colin Ihrig <[email protected]>
All good things come in threes and that's why besides autotools and gyp, you can now also build everyone's favorite platform abstraction library with cmake. This is Ouroboros eating its own tail in a way because cmake depends on libuv, depends on cmake, depends on libuv, depends on... inception! Things it does: * build the shared library * build the static library * install the shared library * install the static library * install the header files * install libuv.pc Things it does not yet do: * build or install the documentation * build or execute the test suite * produce a release tarball * provide auto-config for downstream cmake-based projects Fixes: libuv#1362 PR-URL: libuv#1850 Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: libuv#1850 Reviewed-By: Colin Ihrig <[email protected]>
Member
Author
|
I landed this in 0cdb4a5...1a0f619 to unblock some ongoing work. @mikefero Can you open a PR with your changes so we can discuss over coffee^Wcode? Nothing is set in stone yet so there's still room for bigger changes. |
Contributor
|
Not a problem, I will put something together this weekend (more than likely). Would you prefer them in two separate PRs since one of those commits was not related to CMake (e.g. fixed compile issue with older MSVCs)? |
Member
Author
|
Yes, two PRs would be best. |
Contributor
|
Added a cmake test job to CI - https://ci.nodejs.org/job/libuv-test-commit/934/ |
cjihrig
added a commit
to cjihrig/libuv-release-tool
that referenced
this pull request
Jun 22, 2018
Refs: libuv/libuv#1850 Reviewed-By: Saúl Ibarra Corretgé <[email protected]>
cjihrig
added a commit
to cjihrig/node
that referenced
this pull request
Jun 25, 2018
Notable changes: - Building via cmake is now supported. PR-URL: libuv/libuv#1850 - Stricter checks have been added to prevent watching the same file descriptor multiple times. PR-URL: libuv/libuv#1851 Refs: nodejs#3604 - An IPC deadlock on Windows has been fixed. PR-URL: libuv/libuv#1843 Fixes: nodejs#9706 Fixes: nodejs#7657 - uv_fs_lchown() has been added. PR-URL: libuv/libuv#1826 Refs: nodejs#19868 - uv_fs_copyfile() sets errno on error. PR-URL: libuv/libuv#1881 Fixes: nodejs#21329 - uv_fs_fchmod() supports -A files on Windows. PR-URL: libuv/libuv#1819 Refs: nodejs#12803 PR-URL: nodejs#21466 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos
pushed a commit
to nodejs/node
that referenced
this pull request
Jun 25, 2018
Notable changes: - Building via cmake is now supported. PR-URL: libuv/libuv#1850 - Stricter checks have been added to prevent watching the same file descriptor multiple times. PR-URL: libuv/libuv#1851 Refs: #3604 - An IPC deadlock on Windows has been fixed. PR-URL: libuv/libuv#1843 Fixes: #9706 Fixes: #7657 - uv_fs_lchown() has been added. PR-URL: libuv/libuv#1826 Refs: #19868 - uv_fs_copyfile() sets errno on error. PR-URL: libuv/libuv#1881 Fixes: #21329 - uv_fs_fchmod() supports -A files on Windows. PR-URL: libuv/libuv#1819 Refs: #12803 PR-URL: #21466 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins
pushed a commit
to MylesBorins/node
that referenced
this pull request
Nov 5, 2018
Notable changes: - Building via cmake is now supported. PR-URL: libuv/libuv#1850 - Stricter checks have been added to prevent watching the same file descriptor multiple times. PR-URL: libuv/libuv#1851 Refs: nodejs#3604 - An IPC deadlock on Windows has been fixed. PR-URL: libuv/libuv#1843 Fixes: nodejs#9706 Fixes: nodejs#7657 - uv_fs_lchown() has been added. PR-URL: libuv/libuv#1826 Refs: nodejs#19868 - uv_fs_copyfile() sets errno on error. PR-URL: libuv/libuv#1881 Fixes: nodejs#21329 - uv_fs_fchmod() supports -A files on Windows. PR-URL: libuv/libuv#1819 Refs: nodejs#12803 PR-URL: nodejs#21466 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins
pushed a commit
to nodejs/node
that referenced
this pull request
Nov 11, 2018
Notable changes: - Building via cmake is now supported. PR-URL: libuv/libuv#1850 - Stricter checks have been added to prevent watching the same file descriptor multiple times. PR-URL: libuv/libuv#1851 Refs: #3604 - An IPC deadlock on Windows has been fixed. PR-URL: libuv/libuv#1843 Fixes: #9706 Fixes: #7657 - uv_fs_lchown() has been added. PR-URL: libuv/libuv#1826 Refs: #19868 - uv_fs_copyfile() sets errno on error. PR-URL: libuv/libuv#1881 Fixes: #21329 - uv_fs_fchmod() supports -A files on Windows. PR-URL: libuv/libuv#1819 Refs: #12803 Backport-PR-URL: #24103 PR-URL: #21466 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All good things come in threes and that's why besides autotools and gyp,
you can now also build everyone's favorite platform abstraction library
with cmake.
This is Ouroboros eating its own tail in a way because cmake depends on
libuv, depends on cmake, depends on libuv, depends on... inception!
Things it does:
Things it does not yet do:
Fixes: #1362
The last commit adds support for running tests through ctest(1). It's not super fancy yet but it works.