Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Haz3-jolt
Copy link
Contributor

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

Copy link
Member

@Arthur-Milchior Arthur-Milchior left a comment

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

@Haz3-jolt
Copy link
Contributor 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?

Copy link
Member

@david-allison david-allison left a comment

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
Copy link
Member

@mikehardy mikehardy left a comment

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

@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
Copy link
Member

@mikehardy mikehardy left a comment

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

@mikehardy mikehardy added the Needs Author Reply Waiting for a reply from the original author label Jun 25, 2025
@Haz3-jolt Haz3-jolt removed the Needs Author Reply Waiting for a reply from the original author label Jun 25, 2025
iteration: 0
- api-level: 35
arch: x86_64
target: google_apis
Copy link
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

Copy link
Contributor Author

@Haz3-jolt Haz3-jolt Jun 25, 2025

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
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
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
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