[CI] Fix macOS GitHub CI
ClosedPublic
Actions

Authored by michal on Nov 8 2023, 12:39 PM.

Details

Reviewers

atul
ashoat
tomek

Commits

rCOMM2df5130cab6f: [CI] Fix macOS GitHub CI

Summary

ENG-5678

The general error that breaks the github CI is the same as in this issue - distutils was removed in a newer version of python and so one of the dependencies postinstall script breaks because of that. This diff fixes that by additionaly installing an older (3.10) version of python before installing the dependencies.

Note that I'm still not sure why the CI is sometimes successful but I'm hoping this should fix the errors.

INFO: Most native (written in C++) js modules are built with something called node-gyp. It's written in python and used at build time in postinstall scripts. That's why we need to mess with python.

Test Plan

Tested on a GitHub fork. Without this changes CI failed. After this changes tried running it 3 times and it succeded every time.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

michal created this revision.Nov 8 2023, 12:39 PM

Herald added a subscriber: will. · View Herald TranscriptNov 8 2023, 12:39 PM

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 8 2023, 12:55 PM

Harbormaster failed remote builds in B23900: Diff 32937!

This doesn't seem like a good long-term solution. Did you try either of the other solutions mentioned in the GitHub issue?

I faced a similar issue what I did was install appdmg as a dev dependency and that solved my issue.

Thanks to #2176 (comment) I removed package-lock.json to fix github build action. This is also related to #2176.

Also wondering why the GitHub issue references appdmg, which I don't think appears in our case. Is there perhaps a more relevant GitHub issue that is tracking a long-term solution to this problem? I can't imagine that the Electron team's long-term solution is for devs to lock into an old version of Python.

Separately, I think rebasing should fix the CommTest issue (but not sure since this is a recent diff)

Rebase, instead of downloading an older version of python, we can just download the removed packages.

Did you try either of the other solutions mentioned in the GitHub issue?

I just tried them and they did not work

Also wondering why the GitHub issue references appdmg, which I don't think appears in our case. Is there perhaps a more relevant GitHub issue that is tracking a long-term solution to this problem? I can't imagine that the Electron team's long-term solution is for devs to lock into an old version of Python.

The problem is this:

We use electron-forge package as all-in-one build tool for electron apps
One of the dependencies of electron-forge is electron-rebuild which handles rebuilding any native (C++) modules for the node version of electron we are using (it can't just use the packages in node_modules because they are built for the node on the system and not the node embedded inside electron)
One of dependencies of electron-rebuild is node-gyp which handles the actual building of the native modules
electron-rebuild runs node-gyp on packages inside node_modules (so it's built for the correct node version)

The error happens inside node-gyp and it isn't related to the specific native module that is being built. That's why GitHub issue references a different package than in our case.

The issue has been fixed in node-gyp@10. Unfortunately we are currently using node-gyp@9 and it's a deep dependency so I don't think we can update it ourselves. We will need to wait for electron-rebuild to release a new version using node-gyp@10 (and then for electron-forge to use the newer version of electron-rebuild).

I agree that it's not the best long-term solution but I couldn't find a better one.

michal retitled this revision from [CI] Install python 3.10 for macos to [CI] Fix macOS GitHub CI.Nov 8 2023, 4:27 PM

(Note that this is a draft revision still so I'm not getting emails for your comments)

Came here to link this comment: https://linear.app/comm/issue/ENG-5686/ios-build-ci-failing-on-gh#comment-696c5cbc

We should probably fix all of the builds instead of just this one, as they probably will all have this issue.

it's a deep dependency so I don't think we can update it ourselves

We could try a Yarn resolution if there aren't any breaking changes

Harbormaster completed remote builds in B23911: Diff 32948.Nov 8 2023, 9:48 PM

michal requested review of this revision.Nov 8 2023, 9:48 PM

Can you add this to all of the JS builds?

workflows/android_ci.yml:      - name: yarn ci-cleaninstall
workflows/android_ci.yml:        run: yarn ci-cleaninstall
workflows/android_release.yml:      - name: yarn ci-cleaninstall
workflows/android_release.yml:        run: yarn ci-cleaninstall
workflows/eslint_flow_jest.yml:      - name: yarn ci-cleaninstall
workflows/eslint_flow_jest.yml:        run: yarn ci-cleaninstall
workflows/ios_ci.yml:      - name: yarn ci-cleaninstall
workflows/ios_ci.yml:        run: yarn ci-cleaninstall
workflows/ios_release.yml:      - name: yarn ci-cleaninstall
workflows/ios_release.yml:        run: yarn ci-cleaninstall
workflows/jsi_codegen.yml:      - name: yarn ci-cleaninstall
workflows/jsi_codegen.yml:        run: yarn ci-cleaninstall
workflows/macos_ci.yml:      - name: yarn ci-cleaninstall
workflows/macos_ci.yml:        run: yarn ci-cleaninstall
workflows/remove_harbormaster_tags.yml:      - name: yarn ci-cleaninstall
workflows/remove_harbormaster_tags.yml:        run: yarn ci-cleaninstall
workflows/windows_ci.yml:      - name: yarn ci-cleaninstall
workflows/windows_ci.yml:        run: yarn ci-cleaninstall

This revision is now accepted and ready to land.Nov 9 2023, 11:40 AM

Add the python node-gyp fix to all js CI workflows.

Just making sure, @ashoat, this is what you meant correct?

Yeah that's what I meant!

Harbormaster completed remote builds in B23962: Diff 33014.Nov 9 2023, 3:37 PM

Closed by commit rCOMM2df5130cab6f: [CI] Fix macOS GitHub CI. · Explain WhyNov 10 2023, 1:32 PM

This revision was automatically updated to reflect the committed changes.

Restricted Repository Identity added a commit: rCOMM2df5130cab6f: [CI] Fix macOS GitHub CI.