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

[PragmaTwice]

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]

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]

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

[PragmaTwice]

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]

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]

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. #564 (comment)

[tisonkun]

@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]

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]

@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]

@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]

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]

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]

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

[PragmaTwice]

Solved in #564