quran / quran_android

a quran reading application for android
http://android.quran.com
GNU General Public License v3.0
2.02k stars 892 forks source link

refactor: reorder step of build #2937

Closed MahmoudMabrok closed 2 weeks ago

MahmoudMabrok commented 3 weeks ago

If PR has issues related to lint or test, so after fix we will run build step again with new push event, so I see we can postpone build step which take time and resources until we make sure PR is fine as code quality checks.

ahmedre commented 3 weeks ago

Interesting idea - what if, instead, we split lint out into a different machine, so that both builds run in parallel? This would potentially speed things up for successful builds, and would notify folks of lint issues quicker.

MahmoudMabrok commented 3 weeks ago

for sure, we can do them in parallel (which also will add overhead of new checkout and install dependencies ) but we need to get failure quickly plus we do not need to waste time of build while lint is fail, so with new push that fix lint issues we will build again.

so we would make them sequential to save time of checkout and installing, plus with worst case of lint issue we do not waste time of runners.

github-actions[bot] commented 2 weeks ago
OLD: app-madani-debug.apk (signature: V1, V2)
NEW: app-madani-debug.apk (signature: V1, V2)

          │          compressed          │         uncompressed         
          ├───────────┬───────────┬──────┼───────────┬───────────┬──────
 APK      │ old       │ new       │ diff │ old       │ new       │ diff 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
      dex │  21.3 MiB │  21.3 MiB │  0 B │  68.1 MiB │  68.1 MiB │  0 B 
     arsc │   2.3 MiB │   2.3 MiB │  0 B │   2.3 MiB │   2.3 MiB │  0 B 
 manifest │   5.6 KiB │   5.6 KiB │  0 B │    27 KiB │    27 KiB │  0 B 
      res │   1.6 MiB │   1.6 MiB │  0 B │   1.8 MiB │   1.8 MiB │  0 B 
   native │  18.8 KiB │  18.8 KiB │  0 B │  36.5 KiB │  36.5 KiB │  0 B 
    asset │ 404.2 KiB │ 404.2 KiB │  0 B │ 678.6 KiB │ 678.6 KiB │  0 B 
    other │ 195.9 KiB │ 195.9 KiB │ +5 B │ 404.5 KiB │ 404.5 KiB │  0 B 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
    total │  25.7 MiB │  25.7 MiB │ +5 B │  73.2 MiB │  73.2 MiB │  0 B 
ahmedre commented 2 weeks ago

I think we should keep the order the way it is already, because the compiled code is needed for tests (and, if I am not mistaken, for some lint rules, even if the apk itself isn't required). It also makes more sense. To compare two PR runs around the exact time - one of this PR and one of another:

I am ok extracting lint into a parallel step (along with tests, perhaps), but I don't think it'll save us much in terms of time today, considering we do have to build.

MahmoudMabrok commented 2 weeks ago

I got the point now, but what I was trying to save is time of cases that pr has issues with lint or tests, in these case we will save time for build the app, but seems with current case build step is making overall time is lesser.

at final, we can stick with current implementation (without PR), I had experience that lint and tests fail for many times, so thought to save that time, but seems build step make things faster.

ahmedre commented 2 weeks ago

جزاكم الله خيراً