ci: Build depends only once for Android build #21541

pull MarcoFalke wants to merge 5 commits into bitcoin:master from MarcoFalke:2103-ciAndroid changing 4 files +19 −14
  1. 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

  2. DrahtBot added the label Tests on Mar 28, 2021
  3. icota commented at 5:41 pm on March 28, 2021: contributor

    tACK 04254c746eb162726fde5bcf38c92a23a7a432d6

    Thanks again @MarcoFalke

  4. 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)
  5. hebasto commented at 5:52 pm on March 28, 2021: member

    @MarcoFalke

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

  6. MarcoFalke commented at 5:52 pm on March 28, 2021: member
    no
  7. hebasto commented at 5:53 pm on March 28, 2021: member
    Approach ACK 04254c746eb162726fde5bcf38c92a23a7a432d6
  8. 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
  9. 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
  10. 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
  11. 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
  12. 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
  13. MarcoFalke force-pushed on Mar 28, 2021
  14. MarcoFalke commented at 6:08 pm on March 28, 2021: member
    Added text to commit bodies
  15. 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!

  16. hebasto approved
  17. 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.
  18. 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
    

    ?

  19. fanquake merged this on Mar 29, 2021
  20. fanquake closed this on Mar 29, 2021

  21. sidhujag referenced this in commit 2064024bad on Mar 29, 2021
  22. MarcoFalke deleted the branch on Mar 29, 2021
  23. DrahtBot locked this on Aug 16, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 00:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me