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

v2.7.0 proposal #1622

Closed
zbjornson opened this issue Jul 17, 2020 · 15 comments
Closed

v2.7.0 proposal #1622

zbjornson opened this issue Jul 17, 2020 · 15 comments

Comments

@zbjornson
Copy link
Collaborator

@LinusU @chearon we've got a good chunk of changes in, and no open PRs are ready to land. What do you think of releasing v2.7.0?

GitHub added support for manually triggering GitHub Actions about a week ago. If you guys agree that it's a good time to release, I can update the workflow file and get the builds made.


Changed

  • Switch CI to Github Actions. (Adds Windows and macOS builds.)
  • Switch prebuilds to GitHub actions in the Automattic/node-canvas repository.
    Previously these were in the node-gfx/node-canvas-prebuilt
    and triggered manually.
  • Speed up fillStyle= and strokeStyle=

Added

  • Export rsvgVersion.

Fixed

  • Fix BMP issues. (Fix BMP issues #1497)
  • Update typings to support jpg and addPage on NodeCanvasRenderingContext2D (Updating Types #1509)
  • Fix assertion failure when using Visual Studio Code debugger to inspect Image prototype (Assert failed! Expression: object->internalFieldCount() > 0 #1534)
  • Fix signed/unsigned comparison warning introduced in 2.6.0, and function cast warnings with GCC8+
  • Fix to compile without JPEG support (Build fails if jpeg library not found on Windows 10 x64 #1593).
  • Fix compile errors with cairo
  • Fix Image#complete if the image failed to load.
  • Upgrade node-pre-gyp to v0.15.0 to use latest version of needle to fix error when downloading prebuilds.
  • Don't throw if fillStyle or strokeStyle is set to an object, but that object is not a Gradient or Pattern. (This behavior was non-standard: invalid inputs are supposed to be ignored.)
@Kaleidosium
Copy link

Non-contributor here, but I feel like some of the PRs like the ones by @samizdatco, should be merged first, unless if they say that the PRs still have a long way to go.

@chearon
Copy link
Collaborator

chearon commented Jul 20, 2020

Yeah, it's been a while. Would love to get a release with the new prebuild system.

GitHub added support for manually triggering GitHub Actions about a week ago

Ohh this is great! 🎉

some of the PRs like the ones by @samizdatco, should be merged first

The small-caps one has one small change left. The letter-spacing seems a little controversial (see discussion in #1014) which might be why it hasn't been merged yet. I'm not sure where I stand on that one.

@LinusU
Copy link
Collaborator

LinusU commented Jul 21, 2020

I'm all for releasing as often as possible 🚀

We can always cut a new release after landing the other PRs

If you guys agree that it's a good time to release, I can update the workflow file and get the builds made.

Sounds good! How would this work, should you update something before or after tagging and publishing the release?

@zbjornson
Copy link
Collaborator Author

Agreed on making releases frequently. Want to get this one out since the node-pre-gyp needle issue is still happening for folks.


Sadly prebuilds are still a miserable process. I got Mac and most Linux builds for v2.7.0 up (https://github.com/Automattic/node-canvas/releases), but Windows is failing with an unhelpful "error code 10" when dependency walker is running (log), and uploads for Linux/Node14 are consistently failing with a 500 response from GitHub (log). Giving up for now.

Other notes:

  • Manual workflow trigger inputs can't be used as inputs to the build matrix, so setting which canvas version to build and which Node.js versions to build for still requires adjusting the prebuild yaml file.

  • node-gyp v7 dropped support for Node <10, so I pinned it to v6.1

  • actions/checkout@v2 gives an unhelpful error on Mac and Win ("error code 1") if you forget to push the release tag to GitHub. 🤦

  • GitHub uploads fail on occasion. Need to rerun the build if it happens.

  • numworks/setup-msys2 no longer seems to work, and is replaced by msys2/setup-msys2, but that doesn't have the msys2do script. I updated scripts to use msys2 -c "command". Possibly relates to the Windows build failures.

  • GitHub actions logging is ... minimalist, and without admin access to this repo, we can't enable debug logging.

These changes are in the prebuilds branch.

@chearon
Copy link
Collaborator

chearon commented Aug 3, 2020

@zbjornson that looks like the exit code. It looks like that's happening due to the msys changes you were trying. Dependency Walker's exit code just indicates if some of the dependencies couldn't be resolved statically etc. They don't use it "correctly".

Anyways looks like we have some work to do to use the new msys2 actions. For the failing uploads, maybe we should add retries?

@chearon
Copy link
Collaborator

chearon commented Sep 14, 2020

I did some work on the builds this weekend.

  • Most importantly node-gyp version 8 drops support for node 6-9, so those builds fail and cancel the rest.
  • I got the new msys2/setup-msys2 working
  • The newest Pango changes #include <harfbuzz/hb.h> to #include <hb.h>. That should be as simple as adding the path to harfbuzz to the binding.gyp, but it didn't work. That's where I threw in the towel.

Lesson learned: we need to pin all library version numbers, and node-gyp version.

@piranna
Copy link
Contributor

piranna commented Oct 18, 2020

I have been reviewing all my pull-requests and tests are passing for all of them in all the platforms, can they be reviewed and merged? :-) At least the simple ones...

@zbjornson
Copy link
Collaborator Author

@chearon the Win prebuilds are missing a few libs and I'm not sure why.

When building, these (non MS-WIN) libs are reported by depends.exe:

  • LIBCAIRO-2.DLL, LIBFREETYPE-6.DLL, LIBGIF-7.DLL, LIBGLIB-2.0-0.DLL, LIBGOBJECT-2.0-0.DLL, LIBJPEG-8.DLL, LIBPANGO-1.0-0.DLL, LIBPANGOCAIRO-1.0-0.DLL, LIBPNG16-16.DLL, LIBRSVG-2-2.DLL

and those are copied into the bundle. However, when I download the bundle and open it with depends.exe locally, some additional libraries are listed as missing:

  • zlib1.dll, libcairo-gobject-2.dll, libfontconfig-1.dll, libfribidi-0.dll, libgdk_pixbuf-2.0-0.dll, libgio-2.0-0.dll, libharfbuzz-0.dll, libintl-8.dll, libpangoft2-1.0-o.dll, libpangowin32-1.0-0.dll, libpixman-1-0.dll, libpcre-1.dll, libthai-0.dll, libwinpthread-1.dll, libxml2-2.dll

Of those, the normal (non-prebuild) binding.gyp copies:

  • zlib1.dll, libintl-8.dll, libpangoft2-1.0-0.dll, libpangowin32-1.0-0.dll, libfontconfig-1.dll, libgdk_pixbuf-2.0-0.dll, libxml2-2.dll

These libs exist on the prebuild machine -- it's not that they're missing, it's that depends isn't saying that they need to be copied.

The normal binding.gyp also copies these, not reported by depends.exe:

  • libgmodule-2.0-0.dll, libgthread-2.0-0.dll, libexpat-1.dll, libgio-2.0-0.dll, libcroco-0.6-3.dll, libgsf-1-114.dll, gif.lib

https://github.com/Automattic/node-canvas/runs/1371988925?check_suite_focus=true contains the depends.csv output and a listing of the /mingw64/bin directory.

I'm very tempted to just hard-code the DLLs to copy since that's what the normal binding.gyp files do, but I'd also like to know why this stopped working. When I ported prebuilds to GitHub Actions, they worked fully.

@zbjornson
Copy link
Collaborator Author

I finally got Win prebuilds working this weekend. In the next day or two I'll clean up that work and run the prebuild action for Node 15/canvas 2.6.1, and Node.js all/canvas 2.7.0.

@zbjornson
Copy link
Collaborator Author

Windows and MacOS builds for canvas 2.6.1, Node.js v15 are up.

Linux is failing with this:

/github/home/.cache/node-gyp/15.4.0/include/node/v8.h: In member function 'void v8::TracedReferenceBase<T>::SetSlotThreadSafe(T*)':
/github/home/.cache/node-gyp/15.4.0/include/node/v8.h:912:27: error: 'atomic' in namespace 'std' does not name a template type
...
/github/home/.cache/node-gyp/15.4.0/include/node/v8.h:913:54: error: 'memory_order_relaxed' is not a member of 'std'

@chearon your docker image is based on wheezy (glibc 2.13) and installs gcc 4.7 (and 4.9?). Node.js 12+ requires gcc 6.3+ and glibc 2.17+ (i.e. stretch/9). (Evidently that wasn't a hard requirement until Node.js 15.) Could you update your image sometime please? How long does that take to build -- is it easier to just incorporate that into the prebuild workflow instead?

@chearon
Copy link
Collaborator

chearon commented Dec 16, 2020

Yeah, we'll have to jump to the next version of Debian that has gcc 6.3+/glibc 2.17+, and that will be the new minimum version of glibc since I don't feel like compiling either of those.

How long does that take to build -- is it easier to just incorporate that into the prebuild workflow instead?

You mean like a separate workflow for updating the image? The docker image only needs to be updated when we change gcc/glib/dependency versions, so not sure it makes sense to have it run automatically.

I'll try to get to it soon, need to have updated Pango so I can fix #1572 for prebuilds too.

@zbjornson
Copy link
Collaborator Author

After much struggle, and great help from @chearon, all 2.7.0 builds are up.

We already have stuff for 2.8.0 queuing up, including a few more PRs I'm about to open, but @LinusU I think you can publish 2.7.0 now.

@domoritz
Copy link
Contributor

Congratulations and thank you for working through it! Does this mean #1688 is resolved as well?

@zbjornson
Copy link
Collaborator Author

Oh yeah, got Canvas 2.6.1/Node 15 releases done as well @domoritz 🎉

@LinusU
Copy link
Collaborator

LinusU commented Mar 1, 2021

$ git checkout v2.7.0
HEAD is now at 58bc728 v2.7.0

$ npm publish
+ canvas@2.7.0

🚀 it's live!

Huge thanks to @zbjornson, @chearon, and everyone else involved ❤️

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

No branches or pull requests

6 participants