Skip to content
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

[RunAllTests] Fix #3510: Set up Android SDK hermetically in GitHub Actions to workaround #3024 #3513

Merged
merged 17 commits into from
Jul 22, 2021

Conversation

BenHenning
Copy link
Sponsor Member

@BenHenning BenHenning commented Jul 21, 2021

Fix #3510
Fix part of #2692

Explanation

This PR introduces sufficient CI arrangement to hermetically set up the Android SDK for all Bazel-related workflows. This fixes #3510 by mitigating #3024 by forcing workflows to use an older build tools instead of the latest that's included in the GitHub CI environment. While the hermeticity is nice, it introduces about an extra minute to each Bazel workflow. We may want to consider moving back to GitHub's automatically updating environment longer term once Bazel is sufficiently stable. I've noted this in #1861.

The new setup is being used for both Bazel app builds & tests. Technically, this isn't needed for tests since they don't require dexing, but keeping a consistent and hermetic environment seems ideal for all Bazel-related workflows. Note that we're intentionally not using the new workflow for the script runs since they don't require the Android SDK. That being said, this does result in a CI behavioral change for all related Bazel workflows: it's forcing using SDK 28 instead of 30 or newer.

Note that this also fixes part of #2692 by introducing our first shareable local action & by isolating the common Bazel setup code to it. Note that local actions can only contain shell commands and not reference other actions, so we'll be a bit limited in how much we can share.

Finally, this can be validated by seeing the passing CI checks (particularly, the passing build test). I've also run all the test workflows since they've been changed here, but they're expected to stay passing.

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

@BenHenning BenHenning changed the title Fix #3510: set up Android SDK hermetically in GitHub Actions to workaround #3024 [RunAllTests] Fix #3510: Set up Android SDK hermetically in GitHub Actions to workaround #3024 Jul 22, 2021
This integrates the new action with Bazel tests in addition to the
build. While the tests work fine with the current build tools, we should
be consistent in how the environment is set up for CI workflows.
The new name specifically clarifies why this action isn't needed for the
Bazel-only script runs (since they don't require the Android SDK bits).
@BenHenning BenHenning marked this pull request as ready for review July 22, 2021 01:20
@BenHenning BenHenning requested a review from a team as a code owner July 22, 2021 01:20
@BenHenning BenHenning requested a review from seanlip July 22, 2021 01:20
@BenHenning
Copy link
Sponsor Member Author

@seanlip PTAL for codeowners change.

I'm looking for someone from the Android team to perform a full review, as well.

@BenHenning
Copy link
Sponsor Member Author

@anandwana001 mentioned that he's available to review this--updating assignments.

@seanlip
Copy link
Member

seanlip commented Jul 22, 2021

Sg, thanks. @anandwana001 please reassign me once you've reviewed!

@seanlip seanlip removed their assignment Jul 22, 2021
@anandwana001
Copy link
Contributor

Sure, waiting for CI to pass.

@BenHenning
Copy link
Sponsor Member Author

@anandwana001 CI is passing now--PTAL when you get a chance.

Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

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

As CI is passing, LGTM

@BenHenning
Copy link
Sponsor Member Author

Thanks!

@oppiabot
Copy link

oppiabot bot commented Jul 22, 2021

Unassigning @anandwana001 since they have already approved the PR.

@BenHenning BenHenning merged commit e49edc7 into develop Jul 22, 2021
@BenHenning BenHenning deleted the set-up-hermetic-android-sdk-in-actions branch July 22, 2021 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Required Bazel build CI check now failing for everyone
3 participants