td icon indicating copy to clipboard operation
td copied to clipboard

Propose two new options for building

Open micl2e2 opened this issue 9 months ago • 9 comments

This patch is proposing two new build flags: TD_BUILD_BENCHMARK and TD_BUILD_TESTING, both for the ability of enabling/disabling corresponding submodule's building and running.

The major rationale is that, for some downstream projects that use tdlib as an in-tree dependency(rather than installed one), building the project means building the tdlib, and testing the project means testing the tdlib. While benchmarks and tests are essential submodules, it is not to be distributed to the end users. so in some cases where a downstream has full confidence for the quality of current tdlib's codebase, during a frequent edit-build-debug cycle, it is sensible to disable these two submodules, which could potentially help a third-party app gain a even smoother development experience, which fullfills the very first goal introduces at https://core.telegram.org/tdlib.

Here are some benefits we could gain:

  1. Storage Saving: Considering the storage occupation just under build dir, skipping these two modules would be undoubtedly significant savings:
# Release
...
2M  build/tdutils
45M  build/benchmark
45M  build/test
114M  build/CMakeFiles
404M  build/

# Debug
...
59M  build/tdutils
529M  build/test
869M  build/benchmark
1.3G  build/CMakeFiles
4.9G  build/

And for some downstream projects that use tdlib as an in-tree dependency, the build would be located in those projects' root or somewhere else if their any special preferences.

  1. Time Saving: Building durations of these two are already quite short, but the executions of them are the combination of many cpu-intense, network-dependant tasks, especially for tests:
real	6m47.718s
user	11m33.489s
sys	6m53.766s

at least some minutes required for a relatively performant machine, and they are the cases where cache tools like ccache won't provide any help. These additional time won't be recognized by installed-tdlib downstreams, but in-tree-tdlib downstreams, who have their own tests and also use cmake's testing workflow to run the tests. While CTest already provides a builtin BUILD_TESTING switch, it is for more general cases, switching off it means switching off project's all ctest-based tests, not just tdlib's ones, so a dedicated option would help to customize tests running according to downstreams' special needs. And those new added options are ON by default to make behaviors consistent.

micl2e2 avatar Mar 29 '25 04:03 micl2e2

Hi @levlam! These modifications are ready to be reviewed, could you help take a look?

micl2e2 avatar May 02 '25 13:05 micl2e2

@micl2e2 The proposed changes mostly duplicate the changes proposed in https://github.com/tdlib/td/pull/1127.

levlam avatar May 02 '25 13:05 levlam

Oh, a little bit surprising that there's already similar one being proposed years ago. I should've done some search there.

Effectively, 1127's modifications are almost identical to mine, except one extra switch for tgcli example. Honestly say, I wouldn't choose to add such switch, because downstreams always rely on the successful compilation of all existing sources. But running the tests/benchmarks is just another different situation, adding an option to disable is likely relaxing the burdens for downstreams. And my rationale above is largely based on my practical experience of using tdlib as an in-tree dependency, that using the exactly the same build system could maximize the integration. But I did suffer from the building process because of such missing configurations, during development. It's undoubtedly true they are essential for the release build. But isn't it more true that the frequency of making debug build is way higher than making a release one? And the current settings provide no options to reduce such pain, except we manually edit tdlib's source to disable it or tdlib offering such an built-in option for us.

I think, if they too essential to be offered to disable, they should be included by default(which is already true by this patch). And additionally, we can emphasize their essence by not offering the documents for these switches, or adding some warning comments, or things like that.

-------- Original Message -------- On 5/2/25 21:45, Aliaksei Levin wrote:

levlam left a comment (tdlib/td#3291)

@.***(https://github.com/micl2e2) The proposed changes mostly duplicate the changes proposed in #1127.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

micl2e2 avatar May 02 '25 16:05 micl2e2

The changes from 1127 aren't merged, because for each issue they solve there are much better solutions then adding another configuration parameter.

And my rationale above is largely based on my practical experience of using tdlib as an in-tree dependency,

If you use TDLib in-tree and cmake, you can just include it in CMakeLists.txt as

add_subdirectory(td EXCLUDE_FROM_ALL)

This way only targets explicitly used by your project will be ever built.

But isn't it more true that the frequency of making debug build is way higher than making a release one? And the current settings provide no options to reduce such pain, except we manually edit tdlib's source to disable it or tdlib offering such an built-in option for us.

Debug builds are incremental. Tests and benchmarks are never rebuilt unless changed. They add nearly zero overhead to incremental builds.

levlam avatar May 02 '25 19:05 levlam

EXCLUDE_FROM_ALL sounds like a solution to me, thanks for mentioning, will try it lately to see if it works

micl2e2 avatar May 03 '25 00:05 micl2e2

set_property(DIRECTORY /path/to/tdlib/test PROPERTY EXCLUDE_FROM_ALL ON) should be sufficient to solve the issue I mentioned previously.

micl2e2 avatar May 16 '25 10:05 micl2e2

After several times of trying, it occurs that barely excluding sub directory through option EXCLUDE_FROM_ALL cannot handle the scenario introduced here, it can skip the compilation of sub directory in interest, but CTest still treats the running of tests as a prerequisite, which results in:

88% tests passed, 1 tests failed out of 8

Total Test time (real) =   0.01 sec

The following tests FAILED:
	  1 - run_all_tests (Not Run)
Errors while running CTest

To the best of my knowledge, currently there is no way to conditionally skip these two modules from downstreams through their cmake option, except manually modifying the td's cmake file's content directly.

So I am going to open this again, could you please help take a look? @levlam

micl2e2 avatar Jun 11 '25 03:06 micl2e2