Skip to content

CI test for 16kb and non-16kb page alignment#18248

Open
Haz3-jolt wants to merge 2 commits into
ankidroid:mainfrom
Haz3-jolt:pages
Open

CI test for 16kb and non-16kb page alignment#18248
Haz3-jolt wants to merge 2 commits into
ankidroid:mainfrom
Haz3-jolt:pages

Conversation

@Haz3-jolt

Copy link
Copy Markdown
Member

Purpose / Description

Updates GitHub workflow matrix to extend to 16kb page alignment.

Fixes

Approach

  • Attempts to verify the APK with 16KB page alignment
  • First tries with build-tools v35.0.0, then falls back to v34.0.0 if needed
  • Provides clear success/failure messages

How Has This Been Tested?

Passes CI on fork

Learning (optional, can help others)

Refered to 16KB page alignment docs

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@Arthur-Milchior Arthur-Milchior left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand enough to tell anything else than that we should add a note about the original author of the new file, sorry

Comment thread .github/check_elf_alignment.sh
@Haz3-jolt

Copy link
Copy Markdown
Member Author

@mikehardy
I tried to verify page size with:

      - name: Verify Page Size
        if: matrix.target == 'google_apis_ps16k'
        run: |
          PAGE_SIZE=$(adb shell getconf PAGE_SIZE)
          echo "Page size: $PAGE_SIZE"
          if [ "$PAGE_SIZE" != "16384" ]; then
            echo "Error: Expected page size of 16384, got $PAGE_SIZE"
            exit 1
          fi

But when I noticed it wasn't running in the tests, I realised we can't run adb shell getconf PAGE_SIZE, without an emulator. Is it okay to not include it? I've tried adding it to the Emulator script in here. But It wasn't working.

Is there some other way to test it?

@david-allison david-allison left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executes a Google script. Looks fine to me

@BrayanDSO BrayanDSO added the Needs Second Approval Has one approval, one more approval to merge label May 3, 2025

@mikehardy mikehardy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry this sat so long when you were trying to close an issue I logged myself!

I think all of the ideas and the main work here are great. My comments need to be addressed but I think (hope ?) they're all easy modifications on your core work here, just to future proof it

Comment thread .github/workflows/tests_emulator.yml Outdated
Comment thread .github/workflows/tests_emulator.yml Outdated
Comment thread .github/workflows/tests_emulator.yml Outdated
@mikehardy mikehardy added the Needs Author Reply Waiting for a reply from the original author label Jun 3, 2025
@Haz3-jolt Haz3-jolt requested a review from mikehardy June 4, 2025 12:21
@Haz3-jolt Haz3-jolt added the CI label Jun 5, 2025
@Haz3-jolt Haz3-jolt force-pushed the pages branch 2 times, most recently from dc08ce9 to c44c1cd Compare June 5, 2025 12:55
@david-allison

This comment was marked as resolved.

@Haz3-jolt

This comment was marked as resolved.

@Haz3-jolt Haz3-jolt marked this pull request as draft June 5, 2025 16:20
@Haz3-jolt Haz3-jolt removed the Needs Author Reply Waiting for a reply from the original author label Jun 12, 2025
@Haz3-jolt Haz3-jolt force-pushed the pages branch 2 times, most recently from efccd3e to d46e53c Compare June 25, 2025 14:33

@mikehardy mikehardy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

closer still - but did realize that we're not actually executing tests against the google_apis_ps16k emulator target

Comment thread .github/workflows/tests_emulator.yml Outdated
Comment thread .github/workflows/tests_emulator.yml Outdated
Comment thread .github/workflows/tests_emulator.yml Outdated
@mikehardy mikehardy added the Needs Author Reply Waiting for a reply from the original author label Jun 25, 2025
@Haz3-jolt Haz3-jolt force-pushed the pages branch 2 times, most recently from 727da2f to 9dacc54 Compare June 25, 2025 15:31
@Haz3-jolt Haz3-jolt removed the Needs Author Reply Waiting for a reply from the original author label Jun 25, 2025
Comment thread .github/workflows/tests_emulator.yml Outdated
iteration: 0
- api-level: 35
arch: x86_64
target: google_apis

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic - approved, pending upstream PR merge+release and this change here

Suggested change
target: google_apis
target: google_apis_ps16k

@Haz3-jolt Haz3-jolt Jun 25, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep I have amended the commit already, waiting for the release to do a force push!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had a review comment on that PR I just had time to address now, hopefully will release soon - maintainer there is collaborating fine and that change is needed of course

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hasn't been released yet, but temporarily depending on this commit hash for the android-emulator-runner package should work, the PR is merged now ReactiveCircus/android-emulator-runner@66283c0

@mikehardy mikehardy added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) Blocked by dependency Currently blocked by some other dependent / related change and removed Needs Second Approval Has one approval, one more approval to merge labels Jun 25, 2025
@Haz3-jolt Haz3-jolt marked this pull request as ready for review July 2, 2025 20:09
@mikehardy

Copy link
Copy Markdown
Member

Upstream emulator runner finally released with my PR, should be good to go

@mikehardy

Copy link
Copy Markdown
Member

Dangit. My PR was apparently inadequate, the emulator type of 16kb is now going through but there is some missing piece translating that to the create create avd command

  /usr/bin/sh -c \echo no | avdmanager create avd --force -n test --abi 'google_apis_ps16k/x86_64' --package 'system-images;android-36;google_apis_ps16k;x86_64'  --sdcard '100M'
  Loading local repository...                                                     
  [=========                              ] 25% Loading local repository...       
  [=========                              ] 25% Fetch remote repository...        
  [=======================================] 100% Fetch remote repository...       
  Error: Invalid --tag google_apis_ps16k for the selected package. Valid tags are:
  page_size_16kb

I'm on it

@mikehardy

Copy link
Copy Markdown
Member

This thing just won't get fixed, sigh, now this PR is block

@mikehardy mikehardy added the Blocked by dependency Currently blocked by some other dependent / related change label Nov 25, 2025
@Haz3-jolt

Copy link
Copy Markdown
Member Author

This thing just won't get fixed, sigh, now this PR is block

* [fix: remove problematic / unneeded `--abi` option in avd creation ReactiveCircus/android-emulator-runner#456](https://github.com/ReactiveCircus/android-emulator-runner/pull/456)

CI work being finicky as usual, I think another month or so till this is unblocked

@david-allison

Copy link
Copy Markdown
Member

seeing if this will go through now. Rebased on main

@Haz3-jolt

Copy link
Copy Markdown
Member Author

seeing if this will go through now. Rebased on main

The runner image hasn't had a new release :(

@david-allison

david-allison commented Apr 23, 2026

Copy link
Copy Markdown
Member

@david-allison

Copy link
Copy Markdown
Member

com.ichi2.anki.DeckPickerTest > checkIfStudyOptionsIsDisplayedOnTablet[emulator-5554 - 16] SKIPPED
ERROR | bad color buffer handle 561
emulator-5554 - 16 Tests 2/319 completed. (1 skipped) (0 failed)

com.ichi2.anki.DeckPickerTest > checkIfClickOnCountsLayoutOpensStudyOptionsOnMobile[emulator-5554 - 16] FAILED

@mikehardy

Copy link
Copy Markdown
Member

closing and re-opening to rerun CI because I cannot rerun manually quickly, for unknown reasons

I saw a flake on "checkIfClickOnCountsLayoutOpensStudyOptionsOnMobile" as well in a local run with non-16kb emu so perhaps it is just this test that flakes

@mikehardy mikehardy closed this Jun 7, 2026
@mikehardy mikehardy reopened this Jun 7, 2026
@david-allison

david-allison commented Jun 11, 2026

Copy link
Copy Markdown
Member

I'd merge if someone ran a Unit Test de-flake GitHub Actions run on their branch with these commits in.

Seems good to go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Blocked by dependency Currently blocked by some other dependent / related change CI Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI should test 16kb and non-16kb page alignment in libraries, packaging, and emulator

5 participants