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 CI #182

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

Build CI #182

wants to merge 44 commits into from

Conversation

Kwasow
Copy link
Collaborator

@Kwasow Kwasow commented Jan 15, 2025

After this PR is finished and merged, I advise configuring the repo to require the CI to pass before merging a pull request

GitHub runners (both Linux and macOS) are free for public repositories (docs)

Documentation on how eas builds the apps: Android, iOS

TODO

  • Status badges in README - will be done in separate PR
  • Enable corepack
  • @dansup before merging, please add a repostiroy secret under the name GOOGLE_SERVICES_JSON and add the google-services.json file as it's content

Status

npx expo-doctor on main, before fixes:

✔ Check package.json for common issues
✔ Check Expo config for common issues
✔ Check native tooling versions
✔ Check for common project setup issues
✔ Check dependencies for packages that should not be installed directly
✔ Check if the project meets version requirements for submission to app stores
✔ Check for issues with Metro config
✔ Check npm/ yarn versions
✔ Check for app config fields that may not be synced in a non-CNG project
✔ Check for legacy global CLI installed locally

    ➡ [tamagui] built config and components (222ms):

        Config     ./.tamagui/tamagui.config.cjs
        Components ./.tamagui/tamagui-components.config.cjs
        
✔ Check that native modules do not use incompatible support packages
✔ Check Expo config (app.json/ app.config.js) schema
✖ Check that packages match versions required by installed Expo SDK
✔ Check that native modules use compatible support package versions for installed Expo SDK

Detailed check results:

The following packages should be updated for best compatibility with the installed expo version:
  @shopify/[email protected] - expected version: 1.6.3
  [email protected] - expected version: ~50.0.20
  [email protected] - expected version: 14.1.0
Your project may not work correctly until you install the correct versions of the packages.
Found outdated dependencies
Advice: Use 'npx expo install --check' to review and upgrade your dependencies.

One or more checks failed, indicating possible issues with the project.

Changes

  • Added actions that automatically check if the app builds, if lint passes and if expo-doctor runs without any issues. This means that the majority of patches passing these checks will run on both platforms (functionality is not tested though)
  • Removed android/ and ios/ folders from the repo. This project is managed by expo and these directories are automatically generated during build. I confirmed that their previous contents match what is generated automatically.
  • Downgraded a few dependencies that didn't match what expo expected

Android is built manually using gradlew in the same way it is described in the docs.

iOS is built using xcodebuild which is a bit different from what expo does, but doing it the expo way would require us to load the correct provisioning profile in the CI, which could be done, but would increase the complexity of the setup.

Lint CI will be introduced when the TypeScript transition is complete.

Blockers

Related issues

Closes #165

cache: npm

- name: 📦 Install dependencies
run: npm install
Copy link
Collaborator

Choose a reason for hiding this comment

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

this project uses yarn, so I'd also use that in the ci action?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, but something is wrong with the repo then, because we have a package-lock.json which is an npm file. Yarn uses yarn.lock

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

The CI fails when I use yarn

Copy link
Collaborator

@Simon-Laux Simon-Laux Jan 15, 2025

Choose a reason for hiding this comment

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

yeah I also wondered. but the package manager specified in package.json is yarn:

"packageManager": "[email protected]"

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dansup what's the package management situation, what should we use?

Copy link
Collaborator

@Simon-Laux Simon-Laux Jan 15, 2025

Choose a reason for hiding this comment

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

Maybe we should remove yarn then and say to use npm? the npm lock file was updated recently, so I bet @dansup uses npm meanwhile. 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Simon-Laux I think there is more maintenance work to be done. I believe that google-services.json and the EAS project ID (in app.json) should both be included in the public repo. We can probably keep google-services.json private if @dansup really wants it, but it will be difficult to get around the missing EAS ID in a workflow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think listing/formatting and typescript tests would be already be a good start for ci.
I bet also that you can somehow locally build in GitHub Actions without using expos cloud.

on deltachat project we build iOS in a GitHub action: https://github.com/deltachat/deltachat-ios/actions/runs/12766119235/workflow which works quite well, I bet android might be even easier.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @Simon-Laux, I'll check it out. I tried building it with expo directly, skipping EAS, but I ran into weird issues when running on GitHub. Maybe even more manually is the way to go.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I made an issue for discussing package managers #189

@Kwasow
Copy link
Collaborator Author

Kwasow commented Jan 16, 2025

Lint and Android seem to be done, but I think we want to run npx expo prebuild --platform android (and ios in the iOS build) before the gradlew (xcode) build, but that is not working at the moment.

Edit: it's failing on Github and my laptop, but not on my PC, which is weird.

@Kwasow
Copy link
Collaborator Author

Kwasow commented Jan 16, 2025

Turned out to it was missing LFS. I don't think we need LFS - as part of this PR I'm reversing this project to be properly managed by Expo (meaning the android/ and ios/ directories shouldn't be kept in the repo as Expo generates them on its own).

I know I'm new here, but I'd like to stick around for some time longer. Do you happen to have a different (private) communications channel to discuss issues like these? Would you like me to help out with the mobile part of the project?
@Simon-Laux @dansup

@Kwasow
Copy link
Collaborator Author

Kwasow commented Jan 16, 2025

This PR is ready now @Simon-Laux, waiting for blockers to be merged.

@Kwasow Kwasow changed the title Build+Lint CI [Waiting for blockers] Build+Lint CI Jan 16, 2025
@Simon-Laux
Copy link
Collaborator

Do you happen to have a different (private) communications channel to discuss issues like these?

Same, I'm also new and like to know that, I'm a volunteer just like you ;)

@Kwasow
Copy link
Collaborator Author

Kwasow commented Jan 17, 2025

Same, I'm also new and like to know that, I'm a volunteer just like you ;)

I found out there is a discord server for development here: https://discord.gg/VDhM32hbUK @Simon-Laux. Feel free to join

@Simon-Laux
Copy link
Collaborator

Thanks for the link, I found discord links before, but they were all expired, this one is valid 🤩 thanks again

@Kwasow Kwasow changed the title [Waiting for blockers] Build+Lint CI Build CI Jan 20, 2025
@Kwasow Kwasow marked this pull request as ready for review January 20, 2025 23:34
@Kwasow
Copy link
Collaborator Author

Kwasow commented Jan 21, 2025

Hey @dansup, before merging this, please consider enabling commit squashing (and disabling other forms of merging) in the repo settings - the commit history is a mess (since I could only test by pushing) and it won't look nice if it's included in the commit history of the main branch.

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.

Add CI for code Lint/Checking
2 participants