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.

Sign up for GitHub

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

Jump to bottom

Build Desktop for x64 and arm64 only #8160

Merged
merged 1 commit into from
Mar 22, 2022
Merged

Build Desktop for x64 and arm64 only #8160

merged 1 commit into from
Mar 22, 2022

Conversation

kidroca
Copy link
Contributor

@kidroca kidroca commented Mar 15, 2022 •
edited by roryabraham
Loading

cc @roryabraham

Details

  • Build desktop for 2 mac architectures: x64 and arm64
  • Use different artifact (file) names for each output so it does not mess up hosting
  • Changes don't need to be tested on all platforms because they only change the electron builder config / build script

Fixed Issues

$ #8004

Tests

  • Verify you can still build the Desktop App (npm run desktop-build-staging)
  • Verify you can still run the Desktop App locally (npm run desktop)

PR Review Checklist

Contributor (PR Author) Checklist

  • I verified the PR has a small number of commits behind main
  • 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 clearly indicated the environment tests should be run in (Staging vs Production)
  • I wrote testing steps that cover success & fail scenarios (if applicable)
  • I included screenshots or videos for tests on all platforms
  • I ran the tests & verified they passed on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors related to changes in this PR
  • I followed proper code patterns (see Reviewing the code)
    • I added comments when the code was not self explanatory
    • I put all copy / text shown in the product in all src/languages/* files (if applicable)
    • I followed proper naming convention for platform-specific files (if applicable)
    • I followed style guidelines (in Styling.md) for all style edits I made
    • I followed the JSDocs style guidelines (in STYLE.md)
  • 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 corroborated the UI performance was not affected (the performance is the same than main branch)
  • If I created a new component I verified that a similar component doesn't exist in the codebase

PR Reviewer Checklist

  • I verified the PR has a small number of commits behind main
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the testing environment is mentioned in the test steps
  • I verified testing steps cover success & fail scenarios (if applicable)
  • I checked that screenshots or videos are included for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors related to changes in this PR
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified comments were added when the code was not self explanatory
    • I verified any copy / text shown in the product was added in all src/languages/* files (if applicable)
    • I verified proper naming convention for platform-specific files was followed (if applicable)
    • I verified style guidelines were followed
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components are not impacted by changes in this PR (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 the UI performance was not affected (the performance is the same than main branch)
  • If a new component is created I verified that a similar component doesn't exist in the codebase

QA Steps

  • Verify you can download the x64 version from /NewExpensify.dmg
  • Verify you can download the arm64 version from /NewExpensify-arm64.dmg

Screenshots

Web

Mobile Web

Desktop

Screenshot 2022-03-15 at 18 32 29

Build directory content

Screenshot 2022-03-15 at 18 31 40

iOS

Android

@kidroca
Build for x64 and arm64 only
f69b1b7 
Make the `artifactName` for the `arm64` version: NewExpensify-arm64.dmg
@kidroca kidroca mentioned this pull request Mar 15, 2022
Closed
roryabraham
roryabraham requested changes Mar 15, 2022
View reviewed changes
scripts/build-desktop.sh
@@ -24,4 +24,6 @@ info ""
title "Building Desktop App Archive Using Electron"
info ""
shift 1
"$LOCAL_PACKAGES/electron-builder" --config config/electronBuilder.config.js "$@"
"$LOCAL_PACKAGES/electron-builder" --config config/electronBuilder.config.js --x64 "$@"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we refactor this to run both builds in parallel in the background? I did something similar with the .github/scripts/buildActions.sh that you could use as a reference if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we refactor this to run both builds in parallel in the background?

Running the builds in parallel causes problems

Error: Exit code: 1. Command failed: hdiutil detach -force /dev/disk6
hdiutil: detach failed - No such file or directory

I think it's because during the build the DMG is being mounted and then detached

Perhaps that's why electron-builder builds targets one by one when there are multiple targets in one configuration?

Running in parallel didn't seem to reduce build time much, the error happens about 5-10 sec. before the build would be over

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, thanks for looking into it.

@kidroca kidroca marked this pull request as ready for review March 16, 2022 15:11
@kidroca kidroca requested a review from a team as a code owner March 16, 2022 15:11
@MelvinBot MelvinBot requested review from roryabraham and removed request for a team March 16, 2022 15:27
@roryabraham roryabraham merged commit 6f6eaaa into Expensify:main Mar 22, 2022
@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.

@kidroca kidroca deleted the kidroca/feat/desktop-one-arch-per-build branch March 23, 2022 17:42
@OSBotify OSBotify mentioned this pull request Mar 24, 2022
Closed
69 tasks
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @roryabraham in version: 1.1.46-0 🚀

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

@mvtglobally
Copy link

@roryabraham We validated Desktop - Mac x64 version, it is pass, but unfortunately, we dont have Mac arm64 version to QA. Can maybe someone check internally?

@johnmlee101
Copy link
Contributor

Downloaded and it opened just fine 👍

@PauloGasparSv
Copy link
Contributor

PauloGasparSv commented Mar 28, 2022 •
edited
Loading

Working fine here too! Ran on this machine:
image

@mallenexpensify
Copy link
Contributor

  • Trashed production desktop app
  • downloaded new staging
  • it both downloaded super quick and started SUPER quick.

Seems to be working well, partially because Marc fixed a bug that led to slow loading of chats, it's on staging but not production yet (let me know if you need/want the link)

@Santhosh-Sellavel
Copy link
Collaborator

Working fine for me too!
Screenshot 2022-03-28 at 10 29 55 PM

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @timszot in version: 1.1.46-3 🚀

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

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

@roryabraham roryabraham roryabraham approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@kidroca @OSBotify @mvtglobally @johnmlee101 @PauloGasparSv @mallenexpensify @Santhosh-Sellavel @roryabraham