Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RDY] Revamp the build system #348

Merged
merged 3 commits into from
Mar 22, 2014
Merged

[RDY] Revamp the build system #348

merged 3 commits into from
Mar 22, 2014

Conversation

jszakmeister
Copy link
Contributor

This achieves several goals:

  • Less reliance on scripts so we have better portability to Windows (though we still have a ways to go for proper Windows support). Luajit, luarocks, moonscript, and busted are all installed via CMake
    now.
  • Trying to make use of pkg-config to get the correct libraries. The latest libuv is still broken in this regard, but we'll at least be in a position to use it.
  • Allow the use of Ninja or make. The former runs faster in many environments, and automatically makes use of parallel builds.

This also allows for system installed dependencies--though not through the Makefile just yet--and adds support for FreeBSD. I also made the deps build of libuv and luajit build only the static versions.

Note: we no longer install moonrocks as we were only using install, and that was just adding a single parameter to the luarocks install command. Also, it bumps the minimum required CMake to 2.8.7 since that was the lowest version I had to test with. We could probably push it back further, if required, but it appears many distros have at least that version--RHEL being the notable exception, which appears to use 2.8.4.

Finally, I encountered a few issues along the way. CMake in FreeBSD was being built without SSL support. That has been fixed. Libuv didn't have the pkg-config description right, and now that's fixed in 0.11.22. I discovered a couple of CMake bugs that I still need to report. One being related to type size detection failing (because of a compilation error), but CMake failed to notice and left the define empty. The other being a way to shut off download progress notification when using ExternalProject_Add(). The former doesn't really affect most users, I just happened to run across it. The latter we could work-around by formulating our own file(DOWNLOAD ...) command and running it via ${CMAKE_COMMAND} -P.

I tested this on OS X (Mavericks and Snow Leopard), Debian 7.4, Ubuntu 12.04, and FreeBSD 10.

I believe this ultimately supersedes #198, though we may want to take some elements of #198 (moving the top-level Makefile, and the separate documentation and examples). It also fixes #245 and #232, address #202, #255, and I believe it makes progress toward #328.

One downside of using Ninja: we can't launch the legacy tests through it. It doesn't setup a pseudo-tty, and the tests require it. I do have another branch (https://github.com/jszakmeister/neovim/tree/revamp-vim-tests) that is exploring how to make this better.

@tarruda
Copy link
Member

tarruda commented Mar 12, 2014

Looking good, thanks for this amazing job @jszakmeister

@pusewicz
Copy link

👍

@stefan991
Copy link
Contributor

👍 , but there is a issue with the file downloading part on Travis. The downloads print more than 3000 lines to the log (e.g. https://travis-ci.org/neovim/neovim/jobs/20601268#L826).
Is there an easy way to disable this output?

@jszakmeister
Copy link
Contributor Author

@stefan991 That was one of the bugs I mentioned:

The other being a way to shut off download progress notification when using ExternalProject_Add().

ExternalProject_Add() doesn't provide a way to shut-off the download progress. We can dance around it by calling out to our own cmake script to do the download and extraction. Looks like I should probably go ahead and do that.

@jszakmeister
Copy link
Contributor Author

Ugh. I have something working to avoid the progress junk, but it's a lot uglier than I care for. :-( I want to spend a little more time with it. FWIW, I reported the issue to the CMake team.

@jszakmeister jszakmeister changed the title [WIP] Revamp the build system [RFC] Revamp the build system Mar 20, 2014
@jszakmeister jszakmeister changed the title [RFC] Revamp the build system [RDY] Revamp the build system Mar 20, 2014
@tarruda
Copy link
Member

tarruda commented Mar 21, 2014

@jszakmeister I can see you rebased the commits. Whenever you think it's ready feel free to merge it yourself :)

This achieves several goals:

 * Less reliance on scripts so we have better portability to Windows
   (though we still have a ways to go for proper Windows support).
   Luajit, luarocks, moonscript, and busted are all installed via CMake
   now.
 * Trying to make use of pkg-config to get the correct libraries.  The
   latest libuv is still broken in this regard, but we'll at least be in
   a position to use it.
 * Allow the use of Ninja or make.  The former runs faster in many
   environments, and automatically makes use of parallel builds.

This also allows for system installed dependencies--though not through
the Makefile just yet--and adds support for FreeBSD.

This also make us build libuv and luajit as static libraries only, since
we're only concerned about having static libraries for our bundled
dependencies.
Since libuv.pc is broken at the moment, try to determine libuv's
dependencies ourselves.  This ports most of the checks from libuv into
our CMake build, and fixes the build on other unix platforms.
Underneath the hood, CMake uses libcurl and libcurl has had a number of
issues regarding progress feedback.  In one sample run against Travis
CI, we ended up with nearly 3,000 lines of progress output for a single
download.

Unfortunately, CMake doesn't have the download and extract steps
separate, so we have some extra work that we have to do.  Much of the
content was taken from the ExternalProject.cmake and it's template for
generating the content of the download and extract CMake files.
@jszakmeister jszakmeister merged commit 6639436 into neovim:master Mar 22, 2014
@jszakmeister
Copy link
Contributor Author

@tarruda Thanks. Done.

@tarruda
Copy link
Member

tarruda commented Mar 22, 2014

The build system is looking much better now. Hopefully this will please everyone :)

Great job @jszakmeister, thanks

@mahkoh
Copy link
Contributor

mahkoh commented Mar 22, 2014

make clean && make cmake no longer works.

And why does make build nvim-test?

@jszakmeister
Copy link
Contributor Author

"make clean" doesn't remove the build directory. Instead, it runs the clean target in cmake and cleans the legacy tests. If you'd like a separate target that will remove the build folder (without removing the bundled dependencies), I can do that.

I have a PR up to only build nvim-test when you run unit tests.

@jszakmeister jszakmeister deleted the revamp-build-system branch March 23, 2014 09:46
@lslah
Copy link
Contributor

lslah commented Mar 29, 2014

@jszakmeister

If you'd like a separate target that will remove the build folder (without removing the bundled dependencies), I can do that.

I think this would be a good idea. Currently when you add a new source file, you have to run make distclean && make cmake (or delete build/ manually) due to the cmake-blob-mechanism. make clean && make cmake felt more natural but I could live with another make-target as well.

@jszakmeister
Copy link
Contributor Author

What if we change the cmake target to be something like this:

diff --git a/Makefile b/Makefile
index c20329a..c334ea7 100644
--- a/Makefile
+++ b/Makefile
@@ -32,7 +32,9 @@ all: nvim
 nvim: build/.ran-cmake deps
        +$(BUILD_TOOL) -C build

-cmake: | build/.ran-cmake
+cmake:
+       touch CMakeLists.txt
+       $(MAKE) build/.ran-cmake

 build/.ran-cmake: | deps
        mkdir -p build

That would force the cmake configure process to re-run. Or, we could list the source files instead of globbing, which would cause cmake to reconfigure with a bare make. I prefer the latter, but it can cause a few more conflicts along the way.

@lslah
Copy link
Contributor

lslah commented Mar 30, 2014

@jszakmeister 👍 I think that's a nice solution.

@jszakmeister
Copy link
Contributor Author

@lslah which one? The diff or enumerating the source files? I assume you mean the proposed diff.

@lslah
Copy link
Contributor

lslah commented Mar 30, 2014

@jszakmeister Ah, sorry, somehow overlooked the second option. Yes, I've meant the diff in my last comment.
I've voted for listing the files explicitely in the first place as well, but we've decided to go the other way. It was discussed in #191. Not sure if we should start this discussion again.

Grimy pushed a commit to Grimy/neovim that referenced this pull request Jan 7, 2015
Fix minor inclusion typo in readme
dwb pushed a commit to dwb/neovim that referenced this pull request Feb 21, 2017
This is a rather huge refactor.

Fixes neovim#350 and fixes neovim#348.

It removes 'winnr' from 'maker'.

The behaviour changes so that the output is only processed by windows,
but not buffers.  This means that `Neomake | sp` will not open the
location list, until you go to the previous window.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build building and installing Neovim using the provided scripts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

parallel make doesn't work
7 participants