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

Win arm64 support #46

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

Conversation

salmanmkc
Copy link

@salmanmkc salmanmkc commented Aug 11, 2024

Not sure if this is all that is needed, looks like there's some binding for SQLLite3?

Also would need self-hosted arm agents I think, I don't think GitHub has it yet

@SleepyLeslie
Copy link
Collaborator

Hi @salmanmkc, thanks for your contribution. Unfortunately it seems that GitHub indeed doesn't offer arm64 Windows runners (actions/runner-images#768) at this moment. I don't think Grist Labs plans to connect a self-hosted runner, so we likely couldn't merge this PR as of now. Leaving it to @paulfitz for a decision.

Plus, SQLite3 compatibility is something you must explicitly address. The SQLite3 Node.js binding is architecture-specific. During yarn install, it uses prebuild-install to download a prebuilt binding (see https://www.npmjs.com/package/sqlite3). The hosted builds do not cover Windows arm64, so you will have to build it manually. In fact we are already doing this for Windows ia32:

- name: Fix Windows x86 sqlite3 binding
if: steps.yarn-cache.outputs.cache-hit != 'true' && startsWith(matrix.os, 'windows') && matrix.target == 'x86'
run: yarn upgrade sqlite3
# Prebuilt binding is for x64. We must build from source for x86 target.
# See:
# https://stackoverflow.com/questions/72553650/how-to-get-node-sqlite3-working-on-mac-m1
# https://yarnpkg.com/lang/en/docs/envvars/#toc-npm-config
env:
npm_config_build_from_source: true
npm_config_target_arch: ia32
npm_config_fallback_to_build: true

For some reasons if fallback_to_build is not set it will still download the x64 binding even if target_arch is set to ia32, so we actually manually build the ia32 binding in our workflow.

Furthermore, you will need to resolve the merge conflict caused by #45.

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.

2 participants