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

Use FetchContent instead of ExternalProject and git submodules in CMake #564

Merged
merged 19 commits into from
May 11, 2022

Conversation

PragmaTwice
Copy link
Member

Check #562 and #561 for more details.

@tisonkun

@PragmaTwice PragmaTwice marked this pull request as ready for review May 10, 2022 05:52
@PragmaTwice
Copy link
Member Author

After this PR, the cmake build process will not depend on any files in the externel directory. However, in order to make Makefiles still work, I will restore the externel directory (which is currently deleted in this pr).

@git-hulk
Copy link
Member

Thanks @PragmaTwice. I think we can do that first if it's too complex.

CMakeLists.txt Outdated Show resolved Hide resolved
@tisonkun
Copy link
Member

@PragmaTwice please fix CI failure and ping me when this PR ready. I'll do a manual test on macos env.

@tisonkun
Copy link
Member

@git-hulk it seems that lint-build-test-on-ubuntu fails on test phase, not on build phase, please take a look whether it's about unstable test or stateful test.

@PragmaTwice it seems build-on-ubuntu-with-cmake complains with jemalloc not found.

@git-hulk
Copy link
Member

@git-hulk it seems that lint-build-test-on-ubuntu fails on test phase, not on build phase, please take a look whether it's about unstable test or stateful test.

OK, I had a glance at the failure test case. The root cause should be that case depends on
time sleep, would improve it later.

@git-hulk
Copy link
Member

@PragmaTwice Last failure caused by didn't link -pthread. Can have a test on local with running sh build.sh _build, so you are NOT need to wait for the CI feedback.

@PragmaTwice
Copy link
Member Author

@PragmaTwice Last failure caused by didn't link -pthread. Can have a test on local with running sh build.sh _build, so you are NOT need to wait for the CI feedback.

Sorry, fixed. I didn't encounter this problem when I tested the build locally, probably due to some differences between CI and my local environment.

@PragmaTwice
Copy link
Member Author

@PragmaTwice please fix CI failure and ping me when this PR ready. I'll do a manual test on macos env.

@tisonkun CI has been fixed now.

@tisonkun
Copy link
Member

@PragmaTwice so, how can I build kvrocks with cmake now? You may add a section on README also.

tisonkun
tisonkun previously approved these changes May 11, 2022
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Also manually test on macosx.

Tests failure looks unrelated with this change.

We can later remove submodules and improve build efficiency.

@git-hulk git-hulk requested a review from ShooterIT May 11, 2022 05:47
@ShooterIT
Copy link
Member

I am not familiar with this topic, but it seems a better way.

BTW, i ever wanted to remove Makefile, only keep cmake, since we need to install some libs firstly if we use make.
@git-hulk told me we need a high version cmake which may be not convenient for users. I don't know 3.16 is high? If not, maybe in the future, we can only support cmake.

@git-hulk
Copy link
Member

I am not familiar with this topic, but it seems a better way.

BTW, i ever wanted to remove Makefile, only keep cmake, since we need to install some libs firstly if we use make. @git-hulk told me we need a high version cmake which may be not convenient for users. I don't know 3.16 is high? If not, maybe in the future, we can only support cmake.

Yes, especially on centos, but I'm agree to remove the Makefile since it will cause much extra
work to maintain, as well as can't compatible with FetchContent.

@tisonkun
Copy link
Member

@git-hulk @ShooterIT as stated in #564 (comment), we may proceed this patch to introduce a FetchContent based cmake build system. And remove Makefile build system in a separated PR.

cc @PragmaTwice do I get it right?

@git-hulk
Copy link
Member

Overall looks good, will take a new pass again and have a try on my side tonight.

@PragmaTwice
Copy link
Member Author

PragmaTwice commented May 11, 2022

@git-hulk @ShooterIT as stated in #564 (comment), we may proceed this patch to introduce a FetchContent based cmake build system. And remove Makefile build system in a separated PR.

cc @PragmaTwice do I get it right?

Yeah, I also think it would be good to remove makefiles after this PR.

BTW, i ever wanted to remove Makefile, only keep cmake, since we need to install some libs firstly if we use make. @git-hulk told me we need a high version cmake which may be not convenient for users. I don't know 3.16 is high? If not, maybe in the future, we can only support cmake.

You can easily get cmake binaries from https://github.com/Kitware/CMake/releases (I do not try it in an old distro environment like centos or debian stable, but I do know some glibc linking issue will appear while the glibc version in build environment is higher than it in user's environment). I think the most compatible way to use cmake is to build it by your own (as in build.sh) or use a container.

@PragmaTwice
Copy link
Member Author

I can lower the required cmake version to 3.11 (if it is confirmed that this version contains all the features used), if that helps with compatibility.

@ShooterIT
Copy link
Member

Yeah, I also think it would be good to remove makefiles after this PR.

OK, thanks

I can lower the required cmake version to 3.11 (if it is confirmed that this version contains all the features used), if that helps with compatibility.

If cmake version 3.11 can work well, i vote 3.11

@tisonkun
Copy link
Member

@ShooterIT for choosing a newer version, @PragmaTwice explain in this #564 (comment).

@ShooterIT
Copy link
Member

CMake 3.16.0 is released at Nov 26, 2019
CMake 3.11.0 is released at Dec 01, 2018

no much difference 😂

ShooterIT
ShooterIT previously approved these changes May 11, 2022
@PragmaTwice
Copy link
Member Author

PragmaTwice commented May 11, 2022

I just tested the build process in cmake 3.11, and an error occurred because a feature here in cmake 3.13 is used.

New in version 3.13: install(TARGETS) can install targets that were created in other directories. When using such cross-directory install rules, running make install (or similar) from a subdirectory will not guarantee that targets from other directories are up-to-date. You can use target_link_libraries() or add_dependencies() to ensure that such out-of-directory targets are built before the subdirectory-specific install rules are run.

And cmake 3.13 works well. I will change the required cmake version from 3.16 to 3.13.

cmake/gtest.cmake Outdated Show resolved Hide resolved
@git-hulk
Copy link
Member

@PragmaTwice Cool, Looks good to me. Also tested on my side on MacOSX and Debian.

@PragmaTwice PragmaTwice dismissed stale reviews from ShooterIT and tisonkun via bfdc92f May 11, 2022 10:58
@PragmaTwice
Copy link
Member Author

@ShooterIT The required cmake version has been updated to 3.13. (refer to #564 (comment))

build.sh Outdated Show resolved Hide resolved
Co-authored-by: hulk <hulk.website@gmail.com>
Copy link
Member

@ShooterIT ShooterIT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for your work

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May @git-hulk or @ShooterIT take the honor of merging this patch :)

@git-hulk
Copy link
Member

OK, many thanks for @tisonkun help, I'm very glad to merge and summary.

@git-hulk git-hulk merged commit a51be76 into apache:unstable May 11, 2022
@git-hulk
Copy link
Member

Thanks @PragmaTwice great work again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants