-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[No QA]Add new workflow to validate that HybridApp builds on every commit #55328
Changes from 26 commits
d3bd9f5
45b6c98
8a5620e
b7ad7f0
5a5acf5
7f2f3ab
3622a01
8c5b4ad
05e4a29
9fc9001
621b112
84fbe76
63b5dd0
51e1482
1822133
7ff0979
c64c1f5
c126731
b636925
dcff2a2
ed20005
90e4525
a30b7ed
bb58023
a0b05da
17427b3
0319348
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,138 @@ | ||
name: Verify HybridApp build | ||
|
||
on: | ||
workflow_call: | ||
pull_request: | ||
types: [opened, synchronize] | ||
branches-ignore: [staging, production] | ||
paths: | ||
- '**.kt' | ||
- '**.java' | ||
- '**.swift' | ||
- '**.mm' | ||
- '**.h' | ||
- '**.cpp' | ||
- 'AndroidManifest.xml' | ||
- 'project.pbxproj' | ||
- 'package.json' | ||
AndrewGable marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
concurrency: | ||
group: ${{ github.ref == 'refs/heads/main' && format('{0}-{1}', github.ref, github.sha) || github.ref }}-verify-main | ||
cancel-in-progress: true | ||
|
||
jobs: | ||
verify_android: | ||
name: Verify Android HybridApp builds on main | ||
runs-on: ubuntu-latest-xl | ||
steps: | ||
- name: Checkout | ||
uses: actions/checkout@v4 | ||
with: | ||
submodules: true | ||
ref: ${{ github.event.pull_request.head.sha }} | ||
token: ${{ secrets.OS_BOTIFY_TOKEN }} | ||
# fetch-depth: 0 is required in order to fetch the correct submodule branch | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is very slow, and most likely not necessary (though a manual git fetch may be necessary in a later step). To start, what happens if we just remove this? Also, is "the correct submodule branch" just main in Mobile-Expensify? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's a specific Mobile-Expensify commit that is locked to the latest commit as/when each App deploy is done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that the case currently, or will it only be the case once #55403 is completed? Also, where do we store the current commit in Mobile-Expensify which E/App staging and production should point to? My understanding is that currently all staging and production E/App builds use Mobile-Expensify main, but that there's consensus that this is a problem we need to fix. Am I mistaken? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, on each App staging build we bump the submodule to the latest commit (see every other commit here). The problem is that this leads to Mobile-Expensify changes that can break an App release candidate late in a checklist that can break the build without us realising. To solve that we plan to lock the Mobile-Expensify commit for the full checklist -- but we'll give ourselves the option to override this if we need to. |
||
fetch-depth: 0 | ||
|
||
- name: Update submodule to match main | ||
run: | | ||
git submodule update --init --remote | ||
git fetch | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is also yucky. I feel like what we want is more like this: git submodule update --init --remote --depth 1 |
||
git checkout main | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here, you're checking out the main branch of E/App, right? Is that what we want? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I don't think so. @AndrewGable I think you forgot to do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will include in follow up |
||
|
||
- name: Configure MapBox SDK | ||
run: ./scripts/setup-mapbox-sdk.sh ${{ secrets.MAPBOX_SDK_DOWNLOAD_TOKEN }} | ||
|
||
- name: Setup Node | ||
id: setup-node | ||
uses: ./.github/actions/composite/setupNode | ||
with: | ||
IS_HYBRID_BUILD: 'true' | ||
|
||
- name: Build Android Debug | ||
working-directory: Mobile-Expensify/Android | ||
run: | | ||
if ! ./gradlew assembleDebug | ||
then | ||
echo "❌ Android HybridApp failed to build: Please reach out to Contributor+ and/or Expensify engineers for help in #expensify-open-source to resolve." | ||
exit 1 | ||
fi | ||
|
||
verify_ios: | ||
name: Verify iOS HybridApp builds on main | ||
runs-on: macos-15-xlarge | ||
steps: | ||
- name: Checkout | ||
uses: actions/checkout@v4 | ||
with: | ||
submodules: true | ||
ref: ${{ github.event.pull_request.head.sha }} | ||
token: ${{ secrets.OS_BOTIFY_TOKEN }} | ||
# fetch-depth: 0 is required in order to fetch the correct submodule branch | ||
fetch-depth: 0 | ||
|
||
- name: Update submodule to match main | ||
run: | | ||
git submodule update --init --remote | ||
git fetch | ||
git checkout main | ||
|
||
- name: Configure MapBox SDK | ||
run: ./scripts/setup-mapbox-sdk.sh ${{ secrets.MAPBOX_SDK_DOWNLOAD_TOKEN }} | ||
|
||
- name: Setup Node | ||
id: setup-node | ||
uses: ./.github/actions/composite/setupNode | ||
with: | ||
IS_HYBRID_BUILD: 'true' | ||
|
||
- name: Setup Ruby | ||
uses: ruby/[email protected] | ||
with: | ||
bundler-cache: true | ||
|
||
- name: Install New Expensify Gems | ||
run: bundle install | ||
|
||
- name: Cache Pod dependencies | ||
uses: actions/cache@v4 | ||
id: pods-cache | ||
with: | ||
path: Mobile-Expensify/iOS/Pods | ||
key: ${{ runner.os }}-pods-cache-${{ hashFiles('Mobile-Expensify/iOS/Podfile.lock', 'firebase.json') }} | ||
AndrewGable marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
- name: Compare Podfile.lock and Manifest.lock | ||
id: compare-podfile-and-manifest | ||
run: echo "IS_PODFILE_SAME_AS_MANIFEST=${{ hashFiles('Mobile-Expensify/iOS/Podfile.lock') == hashFiles('Mobile-Expensify/iOS/Manifest.lock') }}" >> "$GITHUB_OUTPUT" | ||
|
||
- name: Install cocoapods | ||
uses: nick-fields/retry@3f757583fb1b1f940bc8ef4bf4734c8dc02a5847 | ||
if: steps.pods-cache.outputs.cache-hit != 'true' || steps.compare-podfile-and-manifest.outputs.IS_PODFILE_SAME_AS_MANIFEST != 'true' || steps.setup-node.outputs.cache-hit != 'true' | ||
with: | ||
timeout_minutes: 10 | ||
max_attempts: 5 | ||
command: npm run pod-install | ||
|
||
- name: Build iOS HybridApp | ||
run: | | ||
# Let us know if the builds fails | ||
set -o pipefail | ||
|
||
# Do not start metro | ||
export RCT_NO_LAUNCH_PACKAGER=1 | ||
|
||
# Build iOS using xcodebuild | ||
if ! xcodebuild \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NABish but maybe create a Fastlane lane for this build command? |
||
-workspace Mobile-Expensify/iOS/Expensify.xcworkspace \ | ||
-scheme Expensify \ | ||
-configuration Debug \ | ||
-sdk iphonesimulator \ | ||
-arch x86_64 \ | ||
CODE_SIGN_IDENTITY="" \ | ||
CODE_SIGNING_REQUIRED=NO \ | ||
CODE_SIGNING_ALLOWED=NO \ | ||
build | xcpretty | ||
then | ||
echo "❌ iOS HybridApp failed to build: Please reach out to Contributor+ and/or Expensify engineers for help in #expensify-open-source to resolve." | ||
exit 1 | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to include
workflow_dispatch
trigger?workflow_call
is for reusable, callable workflows. But we don't have any such usage of this new workflow.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove 👍