rnpgp / rnp

RNP: high performance C++ OpenPGP library used by Mozilla Thunderbird
https://www.rnpgp.org
Other
192 stars 54 forks source link

Improve CI build and test speeds #1571

Closed ronaldtse closed 9 months ago

ronaldtse commented 2 years ago

Our CI builds have increased in build times since adding multiple platforms.

The Windows MSVC builds/tests are especially slow and is becoming a blocking issue for effectively merging PRs.

We need to improve our CI build/test speed.

Thanks to @maxirmx for the following analysis of CI build speeds, we know what to improve one by one.

Workflow Job Total time Build and test Build only Test only
Windows MSVC x64 1h 20m 6m 1h 13m
Windows MSVC Win32 2h 12m 6m 2h 5m
Windows MSYS2 25m 19m
Debian 9 1h 9m 56m
CentOS 7 48m 17m
-- 18m
-- 28m
-- 32m
-- 41m
-- 48m
CentOS 8 1h 6m 32m
-- 41m
-- 45m
-- 55m
-- 56m
macOS 10.15 24m 23m
macOS 11.0 25m
ronaldtse commented 2 years ago

@maxirmx could you please help here? Thanks.

maxirmx commented 2 years ago

It looks like MSVC jobs use Debug configuration Here is an explanation:
https://stackoverflow.com/questions/27086145/what-is-the-default-build-configuration-of-cmake/55993599#:~:text=In%20this%20answer%2C%20it%20says,the%20default%20cmake%20build%20configuration.

I believe I reproduced this issue locally.

ronaldtse commented 2 years ago

@maxirmx is the debug configuration causing tests to run much slower?

ni4 commented 2 years ago

@ronaldtse If there are no optimisations then tests would run slower indeed. Looks like we are hardcoding Debug configuration in the windows-msvc.yml workflow:

cmake -B ./build -G "Visual Studio 16 2019" -A ${{ matrix.arch }} -DCMAKE_TOOLCHAIN_FILE=${{ matrix.vcpkg_dir }}\\scripts\\buildsystems\\vcpkg.cmake -DCMAKE_BUILD_TYPE=Debug .
maxirmx commented 2 years ago

For -G "Visual Studio 16 2019" build type is ignored. cmake --build --config is required

maxirmx commented 2 years ago

@maxirmx is the debug configuration causing tests to run much slower?

Well, it looks like debug configuration doubles test time for MSVC Specifically all "..dsa.." tests are ~10x slower

I changed it, we will see if it helps.

maxirmx commented 2 years ago

debian9 job rebuilds some or all dependencies without cashing I believe python build takes extra 20 minutes or so

ronaldtse commented 2 years ago

Indeed we would want to have some aggressive caching to speed up the CI time.

maxirmx commented 2 years ago

I will do a fork. Otherwise my experiments with GHA will block everybody else

maxirmx commented 2 years ago

Indeed we would want to have some aggressive caching to speed up the CI time.

Debian9 job uses i386 (32 bit) container and is affected by the following bug: https://github.com/actions/checkout/issues/334 both for actions/checkout@v2 and actions/cache@v1 (v2) if run at GHA

There are two possible solutions that can resolve the issue and allow run optimized checkout and aggressive caching:

  1. Rub Debian9 in amd64 container.
  2. Locate and resolve missing dynamic library [== patch action, or container or both]
ronaldtse commented 2 years ago

I'm not even sure why we're only testing Debian 9 but not Debian 10/11.

  1. I think we should run both Debian 9 in i386 and amd64 to ensure tests work on 32-bit systems.
  2. We should try to fix the i386 issue in this case.
ni4 commented 2 years ago

@ronaldtse Debian 9 CI was added to be able to check system with 32-bit time_t

ronaldtse commented 2 years ago

Thanks for the reminder! We should probably test against Debian 10 and 11 as well.

ronaldtse commented 2 years ago

@maxirmx now that #1582 is merged, do you have any further suggestions on how we can improve/streamline the build times?

maxirmx commented 2 years ago

Some thoughts how to reduce debian9 build time by 10 min and bring overall time down to 55-56 min:

So we can do one of the following: 1) Use custom Debian container with cmake and automake preinstalled. We will have to host it at Docker hub (simplest) 2) Host 32-bit cmake and automake distributions somewhere 3) Host 32-bit Node.js distribution somewhere (community friendly since it will help all other developers that are struggling with missing 32-bit actions\cache)

ni4 commented 2 years ago

@maxirmx I was also thinking about docker containers with all dependencies preinstalled (please see https://github.com/rnpgp/rnp/issues/1508) . Those may be published on Github Container Registry for faster download/access speed.

maxirmx commented 2 years ago

@maxirmx I was also thinking about docker containers with all dependencies preinstalled (please see #1508) . Those may be published on Github Container Registry for faster download/access speed.

It looks like deepndencies' setup takes 2 minutes for all platforms except debian.

ronaldtse commented 2 years ago

It looks like deepndencies' setup takes 2 minutes for all platforms except debian.

In this case the additional maintenance burden of docker containers is probably not worthwhile. The caching mechanisms implement are already good enough I suppose.

ni4 commented 2 years ago

It looks like deepndencies' setup takes 2 minutes for all platforms except debian.

Having Docker container has another benefit - we would be safe from periodical failures due to runner or dependencies updates.

maxirmx commented 9 months ago

I believe this issue can be closed now. With a combination of caching and pre-build docker containers we do not do any excessive steps in setup/configure/build tasks