You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/05/09 09:01:30 UTC

[GitHub] [incubator-kvrocks] PragmaTwice opened a new issue, #562: [QUESTION] Consider using `FetchContent` instead of `ExternalProject` and git submodules?

PragmaTwice opened a new issue, #562:
URL: https://github.com/apache/incubator-kvrocks/issues/562

   [`FetchContent`](https://cmake.org/cmake/help/latest/module/FetchContent.html) has been adopted by many projects as a dependency fetching method introduced in newer versions of cmake, for example [the official documentation for gtest](https://google.github.io/googletest/quickstart-cmake.html#set-up-a-project) 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 complexityin development and build management, so there is a trade-off.
   
   I'm happy to provide PRs if that sounds like a good idea.
   
   @tisonkun 
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on issue #562: [NEW] Consider using `FetchContent` instead of `ExternalProject` and git submodules?

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on issue #562:
URL: https://github.com/apache/incubator-kvrocks/issues/562#issuecomment-1121975251

   > 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.
   
   I agree with that. I will try to keep Makefiles working in PR. https://github.com/apache/incubator-kvrocks/pull/564#issuecomment-1121972637


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] tisonkun commented on issue #562: [NEW] Consider using `FetchContent` instead of `ExternalProject` and git submodules?

Posted by GitBox <gi...@apache.org>.
tisonkun commented on issue #562:
URL: https://github.com/apache/incubator-kvrocks/issues/562#issuecomment-1121929416

   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?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on issue #562: [NEW] Consider using `FetchContent` instead of `ExternalProject` and git submodules?

Posted by GitBox <gi...@apache.org>.
git-hulk commented on issue #562:
URL: https://github.com/apache/incubator-kvrocks/issues/562#issuecomment-1123841845

   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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on issue #562: [NEW] Consider using `FetchContent` instead of `ExternalProject` and git submodules?

Posted by GitBox <gi...@apache.org>.
git-hulk commented on issue #562:
URL: https://github.com/apache/incubator-kvrocks/issues/562#issuecomment-1123807490

   > 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](https://github.com/apache/incubator-kvrocks/blob/unstable/CMakeLists.txt#L78). Do you mean that it's NOT obvious enough for developers?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] tisonkun commented on issue #562: [NEW] Consider using `FetchContent` instead of `ExternalProject` and git submodules?

Posted by GitBox <gi...@apache.org>.
tisonkun commented on issue #562:
URL: https://github.com/apache/incubator-kvrocks/issues/562#issuecomment-1123771508

   @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).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on issue #562: [NEW] Consider using `FetchContent` instead of `ExternalProject` and git submodules?

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on issue #562:
URL: https://github.com/apache/incubator-kvrocks/issues/562#issuecomment-1124645215

   Solved in #564


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice closed issue #562: [NEW] Consider using `FetchContent` instead of `ExternalProject` and git submodules?

Posted by GitBox <gi...@apache.org>.
PragmaTwice closed issue #562: [NEW] Consider using `FetchContent` instead of `ExternalProject` and git submodules?
URL: https://github.com/apache/incubator-kvrocks/issues/562


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on issue #562: [NEW] Consider using `FetchContent` instead of `ExternalProject` and git submodules?

Posted by GitBox <gi...@apache.org>.
git-hulk commented on issue #562:
URL: https://github.com/apache/incubator-kvrocks/issues/562#issuecomment-1121956656

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on issue #562: [NEW] Consider using `FetchContent` instead of `ExternalProject` and git submodules?

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on issue #562:
URL: https://github.com/apache/incubator-kvrocks/issues/562#issuecomment-1123870332

   > 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. 
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on issue #562: [NEW] Consider using `FetchContent` instead of `ExternalProject` and git submodules?

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on issue #562:
URL: https://github.com/apache/incubator-kvrocks/issues/562#issuecomment-1123820522

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on issue #562: [NEW] Consider using `FetchContent` instead of `ExternalProject` and git submodules?

Posted by GitBox <gi...@apache.org>.
git-hulk commented on issue #562:
URL: https://github.com/apache/incubator-kvrocks/issues/562#issuecomment-1123879251

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on issue #562: [NEW] Consider using `FetchContent` instead of `ExternalProject` and git submodules?

Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on issue #562:
URL: https://github.com/apache/incubator-kvrocks/issues/562#issuecomment-1121962107

   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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on issue #562: [NEW] Consider using `FetchContent` instead of `ExternalProject` and git submodules?

Posted by GitBox <gi...@apache.org>.
git-hulk commented on issue #562:
URL: https://github.com/apache/incubator-kvrocks/issues/562#issuecomment-1121969108

   Yes, @tisonkun have proposed this in #566 comment, I'm ok to remove the makefile.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] tisonkun commented on issue #562: [NEW] Consider using `FetchContent` instead of `ExternalProject` and git submodules?

Posted by GitBox <gi...@apache.org>.
tisonkun commented on issue #562:
URL: https://github.com/apache/incubator-kvrocks/issues/562#issuecomment-1123830697

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org