Skip to content

New cmakelists #328

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 17 commits into from
Closed

New cmakelists #328

wants to merge 17 commits into from

Conversation

hks2002
Copy link
Contributor

@hks2002 hks2002 commented Mar 14, 2024 •
edited
Loading

After this change:

  1. CMakeLists is easy to read/modify.
  2. CMakeLists is formated by cmake-format.
  3. Build option's values are more deterministic, if not find the libs, it will build faild.
  4. Options TLS_PROVIDER support 'none', 'openssl', 'botan-3', 'auto'.
  5. Provied some scripts to install all dependencies quickly for different OS.
  6. Windows develope become easy(dependencies provide by: vcpkg/conan/cmake_fetchcontent).
  7. Test for windows is working now.

@an-tao an-tao requested a review from marty1885 March 14, 2024 08:27
@hks2002
Copy link
Contributor Author

hks2002 commented Mar 14, 2024

@an-tao please review the code and ignore the source code lint error, just a few space issue.

@an-tao
Copy link
Owner

an-tao commented Mar 14, 2024

It LGTM

marty1885
marty1885 reviewed Mar 14, 2024
View reviewed changes
cmake/BuildDependencies.cmake Outdated
Comment on lines 51 to 69
# For Windows: Prevent overriding the parent project's compiler/linker ⚠️MT/MD⚠️ settings
set(gtest_force_shared_crt
ON
CACHE BOOL "" FORCE
)
FetchContent_MakeAvailable(googletest)

# Set build output path, this is mainly for Windows, to solve the test/unittests running 0x000135 problems
set_standard_build_output(gtest)
set_standard_build_output(gtest_main)
set_standard_build_output(gmock)
set_standard_build_output(gmock_main)
endif()

# spdlog
if(TRANTOR_USE_SPDLOG)
FetchContent_Declare(fmt URL "https://github.com/fmtlib/fmt/archive/refs/tags/10.2.1.tar.gz")
FetchContent_Declare(spdlog URL "https://github.com/gabime/spdlog/archive/refs/tags/v1.13.0.tar.gz")
FetchContent_MakeAvailable(fmt spdlog)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've questions regarding how are we going to maintain these. I have a feeling that without a need, these automatic dependency management will get out of sync with what's available and we won't have the main/brain power to maintain these.

Copy link
Contributor Author

@hks2002 hks2002 Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is mainly for windows, and who don't want to install conan and vcpkg(it not easy to install it in some areas, such china mainland).

Do you know a way to download the latest release version?
Using this https://github.com/randombit/botan/archive/refs/heads/master.zip is the latest develop version.

and using conan needs maintain the libs version also.

Copy link
Owner

Choose a reason for hiding this comment

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

Trantor does not depend on the fmt library.
and if we fetch the source code of the famous libraries and build them into trantor, this can lead some version conflicts with the same libraries in OS used by users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

spdlog needs fmt libray. if using conan or vcpkg, it will download it and build it automaticly.

Copy link
Owner

Choose a reason for hiding this comment

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

Why don't we use the denpendencies mantained by vcpkg or connan.

Copy link
Contributor Author

@hks2002 hks2002 Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why don't we use the denpendencies mantained by vcpkg or connan.

Succesfully install vcpkg is not so easy due to git clone interupt(actully, I didn't install it successful at all).
conan works fine, It just provide a third way.

Copy link
Owner

Choose a reason for hiding this comment

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

Users can use vcpkg or canon to manager dependencies on windows.
for fmt, you could install it by vcpkg (or conan), for example:

.\vcpkg.exe install fmt:x64-windows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Users can use vcpkg or canon to manager dependencies on windows. for fmt, you could install it by vcpkg (or conan), for example:

.\vcpkg.exe install fmt:x64-windows

I know it. if vcpkg installed successfully.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If don't want to use CMAKE FectchContent as an option BUILD_DEPENDENCIES, I can delete these code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Users can use vcpkg or canon to manager dependencies on windows. for fmt, you could install it by vcpkg (or conan), for example:

.\vcpkg.exe install fmt:x64-windows

And add vcpkg.json to this project could let vcpkg works on manifest mode, then just run vcpkg install.
the vcpkg.json is maintenainced at vcpkg ports already.

deps.macos-12.sh Outdated
Comment on lines 1 to 5
#!/bin/bash

# This script is used to install dependencies on MacOS 12.0 or newer
# botan v3
brew install googletest spdlog c-ares openssl@3 botan
Copy link
Collaborator

Choose a reason for hiding this comment

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

DITTO (and other dep installing scripts), how are we going to keep these from getting out-of-date?

@hks2002
Copy link
Contributor Author

hks2002 commented Mar 14, 2024 •
edited
Loading

@marty1885
In Macos, Botan 3.2 works!
image
But failed build when use Botan 3.3 after run brew update
image
image

Sometimes, we can't follow the latest library version, and need control/limit it, do you agree?

@marty1885
Copy link
Collaborator

@hks2002 I'll look into it. It seems to be caused by missing ranges support in Mac's clang.

I introduced Botan as a backup option in case OpenSSL screwed up again and gave up another Heartbleed. And as a platform to stop me from over-designing the TLS backend towards OpenSSL. We don't need to chase the latest version on Botan.

@hks2002
Copy link
Contributor Author

hks2002 commented Mar 14, 2024 •
edited
Loading

@hks2002 I'll look into it. It seems to be caused by missing ranges support in Mac's clang.

I introduced Botan as a backup option in case OpenSSL screwed up again and gave up another Heartbleed. And as a platform to stop me from over-designing the TLS backend towards OpenSSL. We don't need to chase the latest version on Botan.

It is noteworthy that botan have different libarary name in cmake target_link_libraries, conan using 'botan::botan' while vcpkg using 'Botan::Botan'.
So, I write both vcpkg build and conan build in Build windows-msvc workflow actions. Normally, just one is enough, conan is better, conan build all dependencies takes about 15 min, vcpkg build all dependencies takes about 30 min.

@hks2002
Copy link
Contributor Author

hks2002 commented Sep 8, 2024

ref #352,

@hks2002 hks2002 closed this Sep 8, 2024
@hks2002 hks2002 deleted the new_cmakelists branch September 8, 2024 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marty1885 marty1885 marty1885 left review comments

@an-tao an-tao an-tao approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hks2002 @an-tao @marty1885