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

Freemine.cmake.external.proj #29455

Closed
wants to merge 24 commits into from

Conversation

freemine
Copy link
Contributor

@freemine freemine commented Jan 2, 2025

Description

with the hope of clarifying building system of TDengine itself, to isolate source code, external project and building-binaries, so that further work, such as test-cases-running, rebuilding among branches, could be simplied and less error prone.

this is still in early stage, and /contrib directory is still containing both source-code and downloaded external projects and there-built-binaries. but it's believed easy to isolate them later on.

wish you guys might think this potentially useful, and looking forward to your comments.

Checklist

Please check the items in the checklist if applicable.

  • Is the user manual updated?
  • Are the test cases passed and automated?
  • Is there no significant decrease in test coverage?

@freemine
Copy link
Contributor Author

freemine commented Jan 2, 2025

btw, currently, this PR only tested under linux, not including windows or macosx yet.

@freemine
Copy link
Contributor Author

freemine commented Jan 2, 2025

in order to make external project downloaded to be compiled within the main project of TDengine itself with ADD_SUBDIRECTORY, by setting CMAKE__XXX variable and option(YYY), is really not a good practice, because it ruins the global namespace.
SAY, if project A compiles with CMAKE_XXX set to TRUE, while project B compiles with CMAKE_XXX set FALSE,how ugly the directives in contrib/CMakeLists.txt would be?
with ExternalProject_ADD, you can isolate elegantly easy.

截屏2025-01-02 22 04 03

@freemine
Copy link
Contributor Author

freemine commented Jan 2, 2025

截屏2025-01-03 06 43 37

and
截屏2025-01-03 06 45 02

which means: no matter you build the TDengine with -DBUILD_CONTRIB:BOOL=ON or not, taosd likely link with /deps//rocksdb_static/librocksdb.a

this really smells bad.

@tomchon tomchon requested review from stephenkgu, guanshengliang and yihaoDeng and removed request for stephenkgu and guanshengliang January 3, 2025 08:50
@freemine
Copy link
Contributor Author

freemine commented Jan 4, 2025

comparison of built binary images

截屏2025-01-04 08 23 49

@guanshengliang
Copy link
Contributor

These commits could be compiled successfully in my development environment, but there were compilation errors in the CI environment of TDengine. I will contact the maintainer of the CI to see how to handle it. Subsequently, I will carefully review these submissions. However, time is limited before the Spring Festival, so it may not be until February that they can be merged.

@freemine
Copy link
Contributor Author

freemine commented Jan 17, 2025

thx,but I am NOT in a hurry.
actually speaking, this PR is not supposed to be merged directly at the very beginning, it's there to make suggestions to you guys that if it's worth doing so or not.
btw, ci failure is supposed to happen.
there are still several steps to fully fulfill this.

@CLAassistant
Copy link

CLAassistant commented Feb 12, 2025

CLA assistant check
All committers have signed the CLA.

@guanshengliang
Copy link
Contributor

guanshengliang commented Feb 13, 2025

@freemine
代码中对 CMake 脚本的修改思路确实很好。然而,PR 在 macOS 和 Windows 系统下编译链接时时都会遇到问题,我尝试进行了调试,但似乎无法通过简单的修改来解决。具体问题如下:

  1. lz4 和 tz 的下载目录存在冲突,各平台都可能编译失败
  2. rocksdb 的编译选项存在问题,导致在 macOS 和 Windows 系统下编译失败。
  3. 删除 xxhash 文件后,TSZ 在 macOS 和 Windows 系统下编译失败。
  4. 未开启 BUILD_TEST 选项,导致许多单元测试代码未被编译;而开启该选项后,部分单元测试会出现链接失败。

在大家远程交流不方便的情况下,建议不要一次性提交几十个文件(拆分为多个小的 PR),我们小步快跑,也能更快合并

cc @feici02

@freemine
Copy link
Contributor Author

@guanshengliang
thx. and agree.

@freemine freemine mentioned this pull request Feb 23, 2025
3 tasks
@freemine
Copy link
Contributor Author

#29903 is now taking place

@freemine freemine closed this Feb 23, 2025
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