ci: Build depends only once for Android build

MarcoFalke commented at 4:31 pm on March 28, 2021: member

Currently the Android task has several issues:

It is missing a cache instruction, thus failing the build on Cirrus CI
It is running the depends build twice

Fix those issues

DrahtBot added the label Tests on Mar 28, 2021

icota commented at 5:41 pm on March 28, 2021: contributor

tACK 04254c746eb162726fde5bcf38c92a23a7a432d6

Thanks again @MarcoFalke

ryanofsky commented at 5:50 pm on March 28, 2021: member

Code review ACK faf847fa749df2fb7eabf0cb3418ff3fdc02074f. Thanks for fixing this! It was affecting some of my prs (https://github.com/bitcoin/bitcoin/pull/17227#issuecomment-806248409).

In the future since the build system is pretty obscure it would be really helpful if commit messages didn’t just say what was changing but also gave some hint why changes were being made.

faaee1a36f7dc94c5a567c26c1643f17ccf67856 ci: Set DEPENDS_DIR when setting BASE_ROOT_DIR (1/6)
- Change seems great, but is it changing behavior or is it just cleanup? Is there something motivating it or a reason it was written the opposite way previously?
fa7d6ea93d413d5e3e005d2cbbe815b73016a299 ci: Build depends only once for Android build (2/6)
- Again change seems good, but what is it? It seems like it should make things faster, but is just an optimization or also a bug fix?
fae8916f44f9f37bf7a8b3a478f779bceb0f7dc9 ci: Bump Android cross-build to Ubuntu Focal (3/6)
- Is this a bugfix? Just cleanup?
fa768ac9730c7f10274afacb592cd6ce67e4fd06 cirrus: Only cache releases when needed (4/6)
- Bugfix? Cleanup? Optimization?
faf847fa749df2fb7eabf0cb3418ff3fdc02074f cirrus: Add missing depends_sources_cache to Android task (5/6)
- This seems to create a new cache as described https://cirrus-ci.org/guide/writing-tasks/#cache-instruction. Is it just an optimization?

hebasto commented at 5:52 pm on March 28, 2021: member

@MarcoFalke

Are you going to keep the latest “cirrus: temp” commit (04254c746eb162726fde5bcf38c92a23a7a432d6)?

MarcoFalke commented at 5:52 pm on March 28, 2021: member

no

hebasto commented at 5:53 pm on March 28, 2021: member

Approach ACK 04254c746eb162726fde5bcf38c92a23a7a432d6

ci: Set DEPENDS_DIR when setting BASE_ROOT_DIR

The depends dir can not be overwritten by a FILE_ENV file. Also, a FILE_ENV file
might depend on the DEPENDS_DIR value. Thus, set it before reading FILE_ENV.

This commit does not change behavior, but is required for later commits.

Can be reviewed with --color-moved=dimmed-zebra

fa908a41f3

ci: Build depends only once for Android build

Depends is currently built twice for the Android build. For example, the
same task building it twice:

* https://cirrus-ci.com/task/6673185279049728?logs=ci#L3418 (aarch64-linux-android)
* https://cirrus-ci.com/task/6673185279049728?logs=ci#L3422 (x86_64-pc-linux-gnu, 4 lines later)

fac577d423

ci: Bump Android cross-build to Ubuntu Focal

This does not change behavior, but bumping to Focal now means it doesn't
have to be done later when Bionic is no longer used and EOL.

fa97a17ac3

cirrus: Only cache releases when needed

This does not change behavior, but removes unneeded and empty cache
instructions for tasks that don't need them.

ffff4e7373

cirrus: Add missing depends_sources_cache to Android task

This cache entry is required for completeness. The file
src/Makefile.qt.include needs it in this line:
 QT_BASE_PATH = $(shell find ../depends/sources/ -maxdepth 1 -type f -regex ".*qtbase.*\.tar.xz")

This cache entry is tied to the depends_built_cache cache entry. Either
both are present and cached, or neither of them is. Otherwise, the build
will fail.

fa52d7d3ad

MarcoFalke force-pushed on Mar 28, 2021

MarcoFalke commented at 6:08 pm on March 28, 2021: member

Added text to commit bodies

ryanofsky commented at 6:31 pm on March 28, 2021: member

Code review ACK fa52d7d3adc99c0e716628058b4fd083034d27e0. Only change since last review is adding descriptions and changing new RUN_UNIT_TESTS line from true to false. (I assume that change doesn’t do anything because even though prior default was true, it’s a cross compiled build and enabling unit tests would have no effect.)

I’m still not clear if building depends twice before commit 2/5 “ci: Build depends only once for Android build” (fac577d42330e57c17540cabdb8be43c90b715d9) was actually causing problems or just a messy and unnecessary thing to do. But maybe it’s not known. In any case the new descriptions are clear and very helpful!

hebasto approved

hebasto commented at 6:48 pm on March 28, 2021: member

ACK fa52d7d3adc99c0e716628058b4fd083034d27e0, I have reviewed the code and it looks OK, I agree it can be merged after passing CI.

hebasto commented at 7:04 pm on March 28, 2021: member

Maybe also optimize SDK cache:

 0--- a/.cirrus.yml
 1+++ b/.cirrus.yml
 2@@ -33,8 +33,6 @@ global_task_template: &GLOBAL_TASK_TEMPLATE
 3     folder: "/tmp/ccache_dir"
 4   depends_built_cache:
 5     folder: "/tmp/cirrus-ci-build/depends/built"
 6-  depends_sdk_cache:
 7-    folder: "/tmp/cirrus-ci-build/depends/sdk-sources"
 8   ci_script:
 9     - ./ci/test_run_all.sh
10 
11@@ -161,6 +159,8 @@ task:
12 
13 task:
14   name: 'macOS 10.14 [gui, no tests] [focal]'
15+  depends_sdk_cache:
16+    folder: "/tmp/cirrus-ci-build/depends/sdk-sources"
17   << : *GLOBAL_TASK_TEMPLATE
18   container:
19     image: ubuntu:focal
20@@ -185,6 +185,8 @@ task:
21   name: 'ARM64 Android APK [focal]'
22   depends_sources_cache:
23     folder: "/tmp/cirrus-ci-build/depends/sources"
24+  depends_sdk_cache:
25+    folder: "/tmp/cirrus-ci-build/depends/sdk-sources"
26   << : *GLOBAL_TASK_TEMPLATE
27   container:
28     image: ubuntu:focal

?

fanquake merged this on Mar 29, 2021

fanquake closed this on Mar 29, 2021

sidhujag referenced this in commit 2064024bad on Mar 29, 2021

MarcoFalke deleted the branch on Mar 29, 2021

DrahtBot locked this on Aug 16, 2022

ci: Build depends only once for Android build #21541