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

build: combine CMakeLists.txt and cmake.deps/CMakeLists.txt into one #22054

Closed
wants to merge 1 commit into from

Conversation

dundargoc
Copy link
Member

@dundargoc dundargoc commented Jan 30, 2023

Having two separate cmake processes, one for dependencies and one for
neovim, sounds nice in theory but introduces severe limitations that are
hard to work around.

The most important part is that the neovim build and the dependency
build aren't independent builds in reality. functionaltest-lua serves as
a good example. To recreate "make functionaltest-lua" on Windows (or any
other operating system that can't run the Makefile) would require one to
run the following commands:

cmake -S cmake.deps -B .deps -G Ninja -D USE_BUNDLED_LUA=ON
cmake --build .deps
cmake -B build -G Ninja
cmake --build build --target functionaltest-lua

The relationship between USE_BUNDLED_LUA and functionaltest-lua cannot
be established without an external glue such as Makefile, which makes
determining the true dependencies and relationships between the
different parts very difficult. Having a unified, single cmake run would
not only eliminate these types of problems, but also make building
neovim simpler on all platforms.

@github-actions github-actions bot added build building and installing Neovim using the provided scripts dependencies build dependencies (LuaJIT, LibUV, etc.) labels Jan 30, 2023
@dundargoc dundargoc mentioned this pull request Jan 30, 2023
@jamessan
Copy link
Member

Having two separate cmake processes, one for dependencies and one for
neovim, sounds nice in theory but introduces severe limitations that are
hard to work around.
...
Having a unified, single cmake run would
not only eliminate these types of problems, but also make building
neovim simpler on all platforms.

The only hesitation I have with this premise is that it dissolves the line between bundled dependencies and Neovim.

Right now, if I choose to just use CMake to build Neovim (as the Debian package does), I don't have to care at all about the bundled dependencies. Combining the two makes it easier to manage them as a whole, which also potentially makes it easier to forget about/break the non-bundled workflow.

@dundargoc
Copy link
Member Author

Hmm, I agree in part. I personally like the "mental model" of our current setup. First we build the dependencies we need and then we just build neovim. From that perspective there is no real difference from "bundled" or "external" dependencies; they're just dependencies that happen to be in different locations or packaged differently. So I actually agree that this may make reasoning about external dependencies a bit more difficult.

However, I believe we have pretty good safety measures to ensure neovim always works with external dependencies. We already test in our CI if neovim builds correctly with system dependencies on ubuntu. I wouldn't mind adding more CI to tighten up this aspect by also testing if neovim works with non-bundled dependencies on mac or even windows. Testing this is cheap on CI since all dependencies are already pre-built.

@justinmk
Copy link
Member

justinmk commented Feb 2, 2023

To recreate "make functionaltest-lua" on Windows (or any other operating system that can't run the Makefile) would require one to run the following commands:

And after this change, one can skip the cmake -S cmake.deps -B .deps and cmake --build .deps steps. Having to do those steps to change the deps doesn't seem surprising to me. But I can that eliminating them can simplify documentation and removes a place where contributors could get frustrated/confused.

we have pretty good safety measures to ensure neovim always works with external dependencies. We already test in our CI if neovim builds correctly with system dependencies on ubuntu.

Sure, as long as we have that then it's hard to argue that we need the strict separation. The problem with the current setup doesn't seem acute to me, but I'll take your word for it. Hopefully things can be structured in CMakeLists.txt to make the separation very clear.

@zeertzjq also noted:

[combining the steps] may be useful when adding some compile flags to both nvim and dependencies

On the other hand, does it risk confusion for the case where we don't want the same flags for deps? I assume we'll keep the DEPS_CMAKES_ARGS as a separate list?

@dundargoc
Copy link
Member Author

On the other hand, does it risk confusion for the case where we don't want the same flags for deps? I assume we'll keep the DEPS_CMAKES_ARGS as a separate list?

With fetchcontent that might've been a potential problem, but externalproject (for better or worse) completely isolates dependencies from the main build. I don't see any potential risks for this specifically.

More often than not the problem is that dependencies don't use the same flags as the main build.

@dundargoc dundargoc closed this Feb 20, 2023
@dundargoc dundargoc deleted the build/superbuild branch February 20, 2023 19:54
@dundargoc dundargoc restored the build/superbuild branch December 9, 2023 18:58
@dundargoc dundargoc reopened this Dec 9, 2023
@dundargoc dundargoc changed the title build: combine CMakeLists.txt and cmake.deps/CMakeLists.txt into one build: combine CMakeLists.txt and cmake.deps/CMakeLists.txt into one [skip ci] Dec 9, 2023
@dundargoc dundargoc force-pushed the build/superbuild branch 4 times, most recently from eaf9ddd to 2a773ea Compare December 9, 2023 19:39
@dundargoc dundargoc changed the title build: combine CMakeLists.txt and cmake.deps/CMakeLists.txt into one [skip ci] build: combine CMakeLists.txt and cmake.deps/CMakeLists.txt into one Dec 9, 2023
@dundargoc dundargoc force-pushed the build/superbuild branch 2 times, most recently from eb21c5d to 724e509 Compare December 10, 2023 11:50
Having two separate cmake processes, one for dependencies and one for
neovim, sounds nice in theory but introduces severe limitations that are
hard to work around.

The most important part is that the neovim build and the dependency
build aren't independent builds in reality. functionaltest-lua serves as
a good example. To recreate "make functionaltest-lua" on Windows (or any
other operating system that can't run the Makefile) would require one to
run the following commands:

cmake -S cmake.deps -B .deps -G Ninja -D USE_BUNDLED_LUA=ON
cmake --build .deps
cmake -B build -G Ninja
cmake --build build --target functionaltest-lua

The relationship between USE_BUNDLED_LUA and functionaltest-lua cannot
be established without an external glue such as Makefile, which makes
determining the true dependencies and relationships between the
different parts very difficult. Having a unified, single cmake run would
not only eliminate these types of problems, but also make building
neovim simpler on all platforms.
@dundargoc
Copy link
Member Author

This is borderline impossible with cmake. This problem will probably be fixed by #28344

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 dependencies build dependencies (LuaJIT, LibUV, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants