search

apache / kvrocks

Apache Kvrocks is a distributed key value NoSQL database that uses RocksDB as storage engine and is compatible with Redis protocol.
https://kvrocks.apache.org/
Apache License 2.0
3.54k stars 465 forks source link

[NEW] Consider using `FetchContent` instead of `ExternalProject` and git submodules? #562

Closed PragmaTwice closed 2 years ago

PragmaTwice commented 2 years ago

FetchContent has been adopted by many projects as a dependency fetching method introduced in newer versions of cmake, for example the official documentation for gtest already recommends using FetchContent to import itself into other projects (replacing the previous recommended ExternalProject).

The advantage of FetchContent is that it is executed during the cmake configuration phase, which allows the configuration information of dependent projects to be introduced into the main project, unlike ExternalProject, which is executed during the build phase.

This is quite useful especially for dependency projects built with cmake, because the configured cmake target of a dependency project can be used directly in the main project via add_subdirectory (e.g. gtest::gtest), instead of having to manually configure the dependency's header directories, static or shared libraries, compilation options, install directories, etc. and keeping maintaining these redundant information in the main project.

This also makes git submodules unnecessary: by FetchContent, dependencies can be downloaded and configured at the cmake configuration stage, and URLs and commit hash/tag can be flexibly adjusted based on cmake options.

There are NOT a lot of dependencies in kvrocks, and I think using FetchContent to manage dependencies as opposed to ExternalProject will improve the development experience. Of course, introducing a dependency manager such as vcpkg or conan may be a good option, but it introduces additional complexity in development and build management, so there is a trade-off.

I'm happy to provide PRs if that sounds like a good idea.

@tisonkun

tisonkun commented 2 years ago

If this effort can be done, I'm glad to see a lightweight repo to be cloned and we may converge to a unified build system (CMake).

WDYT @git-hulk @ShooterIT?

git-hulk commented 2 years ago

Thanks @PragmaTwice @tisonkun, I think it's very great to make the build process faster. I like this improvement.

PragmaTwice commented 2 years ago

I noticed that this project also maintains handwritten Makefiles, is it acceptable to keep cmake and delete the handwritten Makefiles?

I think keeping only cmake will reduce the maintenance cost.

git-hulk commented 2 years ago

Yes, @tisonkun have proposed this in #566 comment, I'm ok to remove the makefile. But I think we should do this on the other PR.

PragmaTwice commented 2 years ago

Yes, @tisonkun have proposed this in #566 comment, I'm ok to remove the makefile. But I think we should do this on the other PR.

Agree with that. I will try to keep Makefiles working in this PR. https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1121972637

tisonkun commented 2 years ago

@PragmaTwice perhaps you can think of creating a tracking issue for build system changes.

In #564 we replace ExternalProject with FetchContent. As mentioned above, we'd follow up a PR later to remove git submodules and Makefile based build logics.

There is another issue #561 to be addressed in build system settings.

Also, I notice that we don't have a config about CXX_STANDARD used in Kvrocks. It may help for contributors understanding which standard to follow (which features is allowed or welcomed).

git-hulk commented 2 years ago

Also, I notice that we don't have a config about CXX_STANDARD used in Kvrocks. It may help for contributors understanding which standard to follow (which features is allowed or welcomed).

We set the CXX_STANDARD at target compile instruction, like MakeLists.txt#L78. Do you mean that's NOT obvious enough for developers?

PragmaTwice commented 2 years ago

@PragmaTwice perhaps you can think of creating a tracking issue for build system changes.

In #564 we replace ExternalProject with FetchContent. As mentioned above, we'd follow up a PR later to remove git submodules and Makefile based build logics.

There is another issue #561 to be addressed in build system settings.

Also, I notice that we don't have a config about CXX_STANDARD used in Kvrocks. It may help for contributors understanding which standard to follow (which features is allowed or welcomed).

Great idea! I will create a tracking issue about the build system later.

tisonkun commented 2 years ago

@git-hulk Sorry for missing this point. Then it's OK for me. If we encounter further confusion on c++ standard part, we can review the setting style or mention it in doc / readme. I'm OK with current status after you point it out.

git-hulk commented 2 years ago

Got it. Current linter may also frustrate developers since it's not easy to run on local, since it depends on special version of cpplint and cppcheck, but won't discuss here. @PragmaTwice can help to close this issue if planning to create another issue to track the build process change.

PragmaTwice commented 2 years ago

Got it. Current linter may also frustrate developers since it's not easy to run on local, since it depends on special version of cpplint and cppcheck, but won't discuss here. @PragmaTwice can help to close this issue if planning to create another issue to track the build process change.

Ok, I'll close it. BTW, since I have some experience in static analyzer development, I am glad to give some suggestions and enhancements on lint/code analysis.

git-hulk commented 2 years ago

Cool! We are very eager to improve this, thanks dude.

PragmaTwice commented 2 years ago

Solved in #564