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

Add prebuild for macos arm64, enable prebuilds to be run individually. #2451

Open
wants to merge 15 commits into
base: prebuilds
Choose a base branch
from

Conversation

btsimonh
Copy link

@btsimonh btsimonh commented Nov 1, 2024

Thanks for contributing!

  • Have you updated CHANGELOG.md? - no, there is none for prebuild?
  1. adds ability to select the build target. Default is all of '["ubuntu-latest", "windows-latest", "macos-13", "macos-latest"]'.
  2. update to use node 22 since it is the LTS version.
  3. update runners to be from "ubuntu-latest", "windows-latest", "macos-13", "macos-latest".
  4. add some library paths to prebuild/macOS/binding.gyp which enable macos arm64 to build.

for best effect, combine with #2450 to result in smaller prebuilds

I note that 2 tests are failing during prebuild - on the original and on this branch (at least on linux - maybe a font issue). Maybe we should build the libraries from source, as test on main repo is not failing?

Also note #2354 - almost identical changes for actually building on arm64. But I have not included signing. I have never needed to sign a .node or a library before? - but then I don't have an M1/M2 mac.

@zbjornson @jamesbvaughan - pls. comment or combine the two PRs?

@btsimonh
Copy link
Author

btsimonh commented Nov 1, 2024

latest run with both PRs in place here:
https://github.com/btsimonh/node-canvas/actions/runs/11628871040
release files here:
https://github.com/btsimonh/node-canvas/releases/tag/v3.0.0-rc2
(when the workflow completes).

@btsimonh
Copy link
Author

btsimonh commented Nov 1, 2024

ahh... if I test without the container

    container:
      image: ${{ matrix.node < 18 && 'chearon/canvas-prebuilt:9' || 'zbbjornson/canvas-prebuilt:11' }}

in a separate test workflow, it does not fail?

@btsimonh btsimonh marked this pull request as draft November 1, 2024 14:37
@btsimonh btsimonh marked this pull request as ready for review November 1, 2024 16:56
@btsimonh btsimonh mentioned this pull request Nov 1, 2024
1 task
btsimonh and others added 7 commits December 6, 2024 09:22
update node v from 22 to 'latest'

Co-authored-by: Christian Clauss <[email protected]>
update node v from 22 to 'latest'

Co-authored-by: Christian Clauss <[email protected]>
update github actions version

Co-authored-by: Christian Clauss <[email protected]>
update github actions version

Co-authored-by: Christian Clauss <[email protected]>
update node v from 22 to 'latest'

Co-authored-by: Christian Clauss <[email protected]>
update node v from 22 to 'latest'

Co-authored-by: Christian Clauss <[email protected]>
update github action script

Co-authored-by: Christian Clauss <[email protected]>
@btsimonh
Copy link
Author

btsimonh commented Dec 6, 2024

@cclauss - I blindly added all your suggestions :). I don;t see a problem with using nodejs 'latest'. Let's hope you tested the gihub actions version updates (I have one repo which does not like a newer version).

README.md Outdated Show resolved Hide resolved
Co-authored-by: Christian Clauss <[email protected]>
@@ -34,6 +34,8 @@
'<!@(pkg-config pangocairo --libs)',
'<!@(pkg-config freetype2 --libs)',
'<!@(pkg-config librsvg-2.0 --libs)',
'<!@(pkg-config libjpeg --libs)',
'-L/opt/homebrew/lib',
Copy link
Contributor

Choose a reason for hiding this comment

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

Running brew --prefix or echo $HOMEBREW_PREFIX will generate /opt/homebrew on macOS ARM (Apple Silicon) CPUs but will generate /usr/local on Intel Macs. https://docs.brew.sh/Installation

We should support both paths.

canvas_tag: ["v3.0.0-rc2"] # e.g. "v2.6.1"
name: ${{ matrix.canvas_tag }}, Node.js ${{ matrix.node }}, Linux
runs-on: ubuntu-latest
if: contains(fromJson(inputs.os), 'ubuntu-latest')
Copy link
Contributor

Choose a reason for hiding this comment

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

GitHub Actions docs on detecting the operating system: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#detecting-the-operating-system

- if: runner.os == 'Linux'
- if: runner.os == 'macOS'
- if: runner.os == 'Windows'
- if: runner.os != 'Windows'

@cclauss
Copy link
Contributor

cclauss commented Dec 6, 2024

I have one repo which does not like a newer version

To find the current version of any GitHub Action, you can:
transform actions/setup-node@v4 into https://github.com/actions/setup-node/releases

If you have error messages, I will help debug the issue.

change name of macOSarm output to use actual runner.os and runner.arch

Co-authored-by: Christian Clauss <[email protected]>
@chearon
Copy link
Collaborator

chearon commented Dec 7, 2024

I merged #2354 instead since @zbjornson had reviewed it and it was first. OK to close?

@chearon chearon force-pushed the prebuilds branch 11 times, most recently from ee65e2b to 02b26d1 Compare December 7, 2024 19:18
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.

3 participants