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

Build MacOS Desktop from Intel machines in order to support x86 #30402

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

AndrewGable
Copy link
Contributor

Details

This reverts part of my PR here which was trying to speed up the builds we use MacOS runners by using M1 machines. A side effect I did not realize of building on M1 machines is it would default to building a arm64 architecture app, which cannot run on x86 Intel desktop machines. This reverts the change for desktop builds and uses M1 for iOS as there is no side effects there.

Fixed Issues

$ #30396

Tests

  • I will build an AdHoc build and have Applause verify it works on Intel machines

Offline tests

N/A

QA Steps

  • Same as tests

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
    • MacOS: Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

N/A - Build changes only

Android: mWeb Chrome

N/A - Build changes only

iOS: Native

N/A - Build changes only

iOS: mWeb Safari

N/A - Build changes only

MacOS: Chrome / Safari

N/A - Build changes only

MacOS: Desktop

N/A - Build changes only

@AndrewGable AndrewGable self-assigned this Oct 25, 2023
@AndrewGable AndrewGable requested a review from a team as a code owner October 25, 2023 22:39
@melvin-bot melvin-bot bot requested review from MonilBhavsar and removed request for a team October 25, 2023 22:39
@melvin-bot
Copy link

melvin-bot bot commented Oct 25, 2023

@MonilBhavsar Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@AndrewGable
Copy link
Contributor Author

QA confirmed the tests pass

Screenshot 2023-10-25 at 5 02 08 PM

@AndrewGable AndrewGable requested review from marcaaron and removed request for MonilBhavsar October 25, 2023 23:03
@OSBotify
Copy link
Contributor

Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@marcaaron marcaaron merged commit 0f1ad50 into main Oct 25, 2023
20 of 21 checks passed
@marcaaron marcaaron deleted the andrew-desktop-intel branch October 25, 2023 23:21
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify pushed a commit that referenced this pull request Oct 25, 2023
Build MacOS Desktop from Intel machines in order to support x86

(cherry picked from commit 0f1ad50)
@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by https://github.com/marcaaron in version: 1.3.91-3 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@github-actions github-actions bot added the DeployBlockerCash This issue or pull request should block deployment label Oct 26, 2023
@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
App start TTI 1110.038 ms → 1198.698 ms (+88.660 ms, +8.0%) 🔴
Show details
Name Duration
App start TTI Baseline
Mean: 1110.038 ms
Stdev: 30.493 ms (2.7%)
Runs: 1040.4126460002735 1041.3304700003937 1048.2507290001959 1054.1501559996977 1063.4915239997208 1067.7376819998026 1068.502089000307 1073.6285009998828 1078.7832519998774 1079.116170000285 1085.695066999644 1089.2217960003763 1090.5759560000151 1092.6105500003323 1092.7800409998745 1092.920487999916 1094.8552390001714 1096.3082590000704 1096.325582000427 1097.9540200000629 1100.5125070000067 1101.8751450004056 1104.008566999808 1104.9656210001558 1108.1436459999532 1108.9351669996977 1109.4295830000192 1111.300296000205 1111.443745999597 1111.8833029996604 1112.089331000112 1112.7686580000445 1113.8532410003245 1114.5877729998901 1118.51291900035 1118.9919349998236 1121.0329679995775 1121.1497929999605 1123.4650250002742 1123.6209559999406 1123.7899810001254 1124.7703020004556 1126.6424280004576 1128.458651999943 1129.7403819998726 1132.1420799996704 1133.1938060000539 1135.135042999871 1137.77932099998 1146.8773900000378 1146.9719380000606 1149.4699370004237 1154.9439880000427 1155.132058000192 1158.6548830000684 1159.5111499996856 1168.1934049995616 1173.5714549999684

Current
Mean: 1198.698 ms
Stdev: 42.240 ms (3.5%)
Runs: 1090.4656569999643 1103.3150590001605 1113.1735049998388 1126.3910739999264 1134.564565999899 1143.5684099998325 1144.5327119999565 1150.6936949999072 1151.0876039997675 1158.5455809999257 1166.660600000061 1168.8331430000253 1170.3911399999633 1172.8227209998295 1172.8986909999512 1174.5538400001824 1175.2793720001355 1177.7853310001083 1178.504860999994 1180.8709160001017 1182.1537819998339 1182.8060820000246 1182.8837500000373 1188.8127649999224 1190.0559319998138 1192.4242929997854 1195.5308650000952 1197.0593650001101 1197.6645419998094 1199.1480140001513 1200.0665509998798 1200.35937799979 1200.38917999994 1202.0976900001988 1204.1144320000894 1208.403952000197 1209.8015720001422 1213.3098439997993 1215.7980820001103 1220.2192629999481 1221.6401050002314 1222.0710599999875 1229.2739619999193 1230.3458940000273 1232.1032360000536 1232.3247520001605 1232.9186260001734 1235.4987940001301 1237.0580649999902 1237.8937180000357 1241.2100880001672 1244.8435459998436 1245.7819170001894 1247.2528929999098 1252.3228150000796 1252.477361000143 1255.7909909998998 1257.6582220001146 1263.1636290000752 1312.2094999998808

Meaningless Changes To Duration

Show entries
Name Duration
App start runJsBundle 787.183 ms → 826.827 ms (+39.644 ms, +5.0%)
Open Search Page TTI 708.766 ms → 714.130 ms (+5.364 ms, +0.8%)
App start regularAppStart 0.014 ms → 0.015 ms (+0.000 ms, +2.7%)
App start nativeLaunch 22.241 ms → 21.845 ms (-0.396 ms, -1.8%)
Show details
Name Duration
App start runJsBundle Baseline
Mean: 787.183 ms
Stdev: 27.335 ms (3.5%)
Runs: 710 729 730 738 752 756 756 759 759 760 761 764 765 765 768 768 768 772 772 775 776 776 782 784 784 788 788 789 789 790 790 791 791 793 794 795 795 795 796 796 798 802 802 805 806 807 808 810 811 812 816 816 819 825 825 826 826 828 835 845

Current
Mean: 826.827 ms
Stdev: 23.036 ms (2.8%)
Runs: 770 781 789 789 798 804 806 807 808 809 810 810 810 812 813 813 815 816 817 818 818 820 821 823 826 826 829 829 831 832 832 833 833 833 834 836 836 837 837 838 839 843 847 851 852 854 855 860 864 868 878 885
Open Search Page TTI Baseline
Mean: 708.766 ms
Stdev: 45.607 ms (6.4%)
Runs: 641.9687090003863 642.9658620003611 643.1021729996428 644.1144610000774 645.9055989999324 651.3049730006605 651.8407800002024 655.4641929995269 662.1476649995893 662.4947099993005 663.7545170001686 664.0597740001976 667.734333999455 667.887085000053 668.1236580004916 668.7886560000479 669.9974770005792 671.056599999778 671.1382240001112 676.5769859999418 677.3428960004821 681.7709969999269 682.5491140000522 687.9806320006028 691.8615720001981 697.3706060005352 698.2661950001493 699.2668049996719 700.540690000169 702.6333419997245 703.5332039995119 707.6474210005254 717.58390300069 718.7061359994113 719.1125900000334 719.7654630001634 721.2747400002554 721.3825690001249 723.3932290002704 724.7067460007966 725.4331060005352 725.4905999992043 727.6905119996518 729.4044599998742 734.4562990004197 735.3306080000475 739.4937339993194 740.5779629992321 741.3775639999658 751.5603439994156 754.4057620000094 755.8371179997921 767.0033780001104 770.6504309996963 776.6761070005596 776.8514410005882 777.6168219996616 782.1171880001202 788.4953209999949 822.0659600002691 825.0712489997968

Current
Mean: 714.130 ms
Stdev: 40.951 ms (5.7%)
Runs: 640.62552900007 641.463339000009 648.1735029998235 658.7992760003544 663.2858070000075 663.7485770001076 664.1915690000169 664.353311999701 665.2061769999564 668.1292320000939 668.1299239997752 675.1718759997748 675.6216229996644 679.1371259996668 680.3803310003132 684.0386969996616 685.4532059999183 685.5112310000695 687.0387380002066 689.195395000279 690.8433030000888 691.4321699999273 692.6059570000507 693.140625 694.2627770002 696.6015630001202 700.0322670000605 702.5708820000291 708.7259930004366 710.8240970000625 712.9365240000188 713.31042600004 722.4191080001183 724.0635580001399 725.2655850001611 727.7977299997583 731.7409270000644 731.8075769999996 733.8870040001348 734.5905770002864 734.5950929997489 736.1096600000747 737.1287839999422 737.5877690003254 739.7465010001324 744.9942630003206 747.3892010003328 749.5579020003788 753.3698330000043 754.2399089997634 754.8321940000169 755.4055989999324 757.8661699998192 757.9851890001446 758.4241949999705 762.1330160000362 773.1825769999996 774.2234300002456 775.9262700001709 790.00415099971 840.7387290000916
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (5.8%)
Runs: 0.0125730000436306 0.012899000197649002 0.0129399998113513 0.01297999918460846 0.013061000034213066 0.013061000034213066 0.013224000111222267 0.013386999256908894 0.013387000188231468 0.013427999801933765 0.0134680001065135 0.0134680001065135 0.013591000810265541 0.013671000488102436 0.013794000260531902 0.013794000260531902 0.0138349998742342 0.013876000419259071 0.013957000337541103 0.013996999710798264 0.013996999710798264 0.0139979999512434 0.0139979999512434 0.01407800056040287 0.014159999787807465 0.014241000637412071 0.014241000637412071 0.014241999946534634 0.014282000251114368 0.014322999864816666 0.014322999864816666 0.014363999478518963 0.014364000409841537 0.014444999396800995 0.01444500032812357 0.014526999555528164 0.014566999860107899 0.01460800040513277 0.01460800040513277 0.014649000018835068 0.014689000323414803 0.0147299999371171 0.014771000482141972 0.014810999855399132 0.014851999469101429 0.0148930000141263 0.014973999932408333 0.01501499954611063 0.015054999850690365 0.015056000091135502 0.015096000395715237 0.015217999927699566 0.015219000168144703 0.015300000086426735 0.015747000463306904 0.015991000458598137 0.016193999908864498 0.016479000449180603

Current
Mean: 0.015 ms
Stdev: 0.001 ms (6.3%)
Runs: 0.012980000115931034 0.013021000195294619 0.013265000190585852 0.013265000190585852 0.013428000267595053 0.013590000104159117 0.013590000104159117 0.013590999878942966 0.013631000183522701 0.01375299971550703 0.013874999713152647 0.01399700017645955 0.014078999869525433 0.01407900033518672 0.01407900033518672 0.014160000253468752 0.014200999867171049 0.014283000025898218 0.014283000025898218 0.0143630001693964 0.014403999783098698 0.014404000248759985 0.014404000248759985 0.014404999557882547 0.014405000023543835 0.014444999862462282 0.014444999862462282 0.01444500032812357 0.014485999941825867 0.014607999939471483 0.01460800040513277 0.014688999857753515 0.014770000241696835 0.014810999855399132 0.014851999934762716 0.014851999934762716 0.014973999932408333 0.015014000236988068 0.015015000011771917 0.015015000011771917 0.015055000316351652 0.01509599993005395 0.015137000009417534 0.015178000088781118 0.015298999845981598 0.015625 0.015705999918282032 0.015746999997645617 0.01582799991592765 0.015868999995291233 0.015949999913573265 0.016194999683648348 0.01623600022867322 0.016398000065237284 0.016927000135183334 0.017089999746531248
App start nativeLaunch Baseline
Mean: 22.241 ms
Stdev: 2.090 ms (9.4%)
Runs: 19 19 19 19 20 20 20 20 20 20 21 21 21 21 21 21 21 21 21 21 22 22 22 22 22 22 22 22 22 22 22 22 22 22 22 22 23 23 23 23 23 24 24 24 24 24 24 24 26 26 26 26 28 28

Current
Mean: 21.845 ms
Stdev: 2.839 ms (13.0%)
Runs: 18 18 18 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 20 20 21 21 21 21 21 21 21 21 22 22 22 22 22 22 22 22 23 23 23 23 23 23 23 24 25 26 26 26 26 27 27 28 28 28 28

@github-actions
Copy link
Contributor

@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker.

@marcaaron
Copy link
Contributor

I think this is a false positive. I don't see why this would affect performance. 🤷

@marcaaron marcaaron removed the DeployBlockerCash This issue or pull request should block deployment label Oct 26, 2023
@mvtglobally
Copy link

@marcaaron are we good to check this off the checklist?

@marcaaron
Copy link
Contributor

Yep!

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/marcaaron in version: 1.3.91-8 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.93-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.93-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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

Successfully merging this pull request may close these issues.

4 participants