-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Support skipping non-essential parts of the build #1127
base: master
Are you sure you want to change the base?
Conversation
This significantly speeds up build times for projects which just want to use tdlib as-is. On a single core this should cut about 5 minutes; on 4 cores it cuts a bit more than a minute. All this for no measurable cost. Without skipping: $ cmake -DCMAKE_BUILD_TYPE=Release -DTD_SKIP_BENCHMARK=OFF -DTD_SKIP_TEST=OFF -DTD_SKIP_TG_CLI=OFF .. ( ... regular output ... ) $ time /usr/bin/time make install -j4 DESTDIR=../../td_destdir ( ... regular output ... ) 1414.69user 55.66system 7:01.60elapsed 348%CPU (0avgtext+0avgdata 2752144maxresident)k 0inputs+1210120outputs (0major+21767636minor)pagefaults 0swaps real 7m1,604s user 23m34,699s sys 0m55,665s With skipping: $ cmake -DCMAKE_BUILD_TYPE=Release -DTD_SKIP_BENCHMARK=ON -DTD_SKIP_TEST=ON -DTD_SKIP_TG_CLI=ON .. ( ... regular output ... ) $ time /usr/bin/time make install -j4 DESTDIR=../../td_destdir ( ... regular output ... ) 1118.16user 42.62system 5:56.22elapsed 325%CPU (0avgtext+0avgdata 2751196maxresident)k 0inputs+891912outputs (0major+17310062minor)pagefaults 0swaps real 5m56,230s user 18m38,163s sys 0m42,629s
This reduces build time only by 15% in your example. I'm not sure that such small speed up worth build setup complication, especially given build time can be improved much more without any change. You need to build TDLib only once, so build time should be irrelevant for ordinary usages, and for development purposes it is important to check that everything is built correctly. I also wouldn't call tests a non-essential part. To build a specific target it is better to explicitly specify it, for example, |
I don't really see how this is a complication.
It saves 5 minutes on Travis every time we need to rebuild. (Also because Travis only goes to
You're right, I shouldn't have called it non-essential. However, I hope you can see that someone who just wants to install it (and not run tests or benchmarks) may want to cut down those 5 minutes.
It's also very brittle: This would mean that we ignore td's build system, and "guess" what the Effectively I would need to check every now and then whether our "guess" is still "correct enough", which would be a non-automatable process. |
You can try |
What? Is that a serious comment? How does this solve anything? |
If it is not properly documented, then almost noone will use it, so it is useless. If it is properly documented, then it is a complication, because everyone will need to read about it.
Unless you are going to test TDLib build or run TDLib tests, there is no reason to rebuild TDLib every time. It is much simpler and efficient to create a Docker container with prebuilt TDLib or use https://docs.travis-ci.com/user/caching/#caching-directories-bundler-dependencies. I thought a few times about conditional building of some TDLib parts, but all the time when this was an acceptable solution, there was a simple way to achieve better result. |
Code |
Hii |
CMake options have built-in documentation, having like 8 extra lines in the CMake file to reduce every build by 15% seems a pretty good deal to me... Also, not everyone needs to know about them, just those who look through the list of options if they want to customize the build. The CMake file already has a bunch of options so I'm not sure why adding 3 more to speed up compilation adds complexity if I must be honest |
@mbasaglia If you know that you can see CMake options, you should also know that you can build a single target with CMake.
That's exactly the reason why they will be useless. 1% of developers will do that, but even these developers should be able to customize CMakeLists.txt themselves in no time. |
Customizing CMakeLists.txt in CI is not viable, also I would need to know which targets are needed... |
|
I need that options, please merge them. |
@eugenebas As explained in #1127 (comment) and #1127 (comment), if you need these options, you are doing something wrong. What is you use case? |
It would be nice to have benchmark and test projects DISABLED by default and parallel building ENABLED! It's 2022 and people use multicore systems mostly. |
@andrew-phi Enabled multicore build by default would lead only to failed build by default. If you are sure that you have enough RAM for multicore build, the you are free to pass corresponding argument to build system. |
@levlam if it would fail, it would fail faster than waiting many hours just to get really, REALLY frustrated (I waited ~3h for 70%, 4cores/6GB). I'm not familiar with cmake, but |
This is exactly the command, which must not be advertised. GCC needs about 4GB per core, so the mentioned command will trigger OOM, killing random processes on your server. It have no chance to succeed with 6GB RAM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move option
s to one place
See: #2504 (comment) |
This significantly speeds up build times for projects which just want to use tdlib as-is.
On a single core this should cut about 5 minutes; on 4 cores it cuts a bit more than a minute.
All this for no measurable cost.
Without skipping:
With skipping: