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

Near-perfect font selection on Linux #1987

Merged
merged 2 commits into from Feb 26, 2022
Merged

Conversation

chearon
Copy link
Collaborator

@chearon chearon commented Feb 10, 2022

Finally done. Removes some GLib because it ended up making that code easier. Also updates the prebuild image so the next prebuilds will have improved matching too.

Fixes #1572

.github/workflows/prebuild.yaml Outdated Show resolved Hide resolved
src/register_font.cc Show resolved Hide resolved
@avra-m3
Copy link

avra-m3 commented Feb 14, 2022

Hey @chearon, not sure if this PR is the cause but canvas no longer builds on alpine.
using node:16-alpine as the base for the dockerfile.

make: Entering directory '<project dir>/node_modules/canvas/build'
#12 68.73   SOLINK_MODULE(target) Release/obj.target/canvas-postbuild.node
#12 68.73   COPY Release/canvas-postbuild.node
#12 68.73   CXX(target) Release/obj.target/canvas/src/backend/Backend.o
#12 68.73   CXX(target) Release/obj.target/canvas/src/backend/ImageBackend.o
#12 68.73   CXX(target) Release/obj.target/canvas/src/backend/PdfBackend.o
#12 68.73   CXX(target) Release/obj.target/canvas/src/backend/SvgBackend.o
#12 68.73   CXX(target) Release/obj.target/canvas/src/bmp/BMPParser.o
#12 68.73   CXX(target) Release/obj.target/canvas/src/Backends.o
#12 68.73   CXX(target) Release/obj.target/canvas/src/Canvas.o
#12 68.73   CXX(target) Release/obj.target/canvas/src/CanvasGradient.o
#12 68.73   CXX(target) Release/obj.target/canvas/src/CanvasPattern.o
#12 68.73   CXX(target) Release/obj.target/canvas/src/CanvasRenderingContext2d.o
#12 68.73   CXX(target) Release/obj.target/canvas/src/closure.o
#12 68.73   CXX(target) Release/obj.target/canvas/src/color.o
#12 68.73   CXX(target) Release/obj.target/canvas/src/Image.o
#12 68.73   CXX(target) Release/obj.target/canvas/src/ImageData.o
#12 68.73   CXX(target) Release/obj.target/canvas/src/init.o
#12 68.73   CXX(target) Release/obj.target/canvas/src/register_font.o
#12 68.73 ../src/register_font.cc: In function 'char* get_family_name(FT_Face)':
#12 68.73 ../src/register_font.cc:145:18: error: 'string' is not a member of 'std'
#12 68.73   145 |             std::string best_buf_modified = "@";
#12 68.73       |                  ^~~~~~
#12 68.73 ../src/register_font.cc:21:1: note: 'std::string' is defined in header '<string>'; did you forget to '#include <string>'?
#12 68.73    20 | #include FT_TRUETYPE_IDS_H
#12 68.73   +++ |+#include <string>
#12 68.73    21 | #ifndef FT_SFNT_OS2
#12 68.73 ../src/register_font.cc:146:13: error: 'best_buf_modified' was not declared in this scope
#12 68.73   146 |             best_buf_modified += buf;
#12 68.73       |             ^~~~~~~~~~~~~~~~~
#12 68.73 make: *** [canvas.target.mk:158: Release/obj.target/canvas/src/register_font.o] Error 1

@avra-m3
Copy link

avra-m3 commented Feb 14, 2022

Rolling back the best_buf_modified code to the version you had previously allows you to buiild in alpine, see here avra-m3@e07acd2

@chearon chearon force-pushed the ch/1572 branch 2 times, most recently from e51c9d8 to 43c0f51 Compare February 14, 2022 15:48
@chearon
Copy link
Collaborator Author

chearon commented Feb 14, 2022

@avra-m3 it should work now (43c0f51)

@brosenquist
Copy link

@chearon I'm wondering when this can be merged. Looks like the lint is still failing. Is that what is preventing merge?

@chearon
Copy link
Collaborator Author

chearon commented Feb 24, 2022

It's just awaiting approval from someone else. Lint is failing on master and is fixed by #1986.

@brosenquist
Copy link

brosenquist commented Feb 25, 2022

@chearon Thanks for the update. Looks like #1986 was merged. Yay!
Sorry if this is a dumb question. I'm not familiar with the process for getting PR approval. Can you tag someone to review/approve or do you just have to wait for someone to decide they want to do it? I do see that @zbjornson is assigned as reviewer.
I don't mean to be a nag, but as you can probably tell I'm really excited to get this fix in. It will solve a bunch of problems for us. Thanks so much for making the fix!

Copy link
Collaborator

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

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

Looks good 👍. Just needs the merge conflict fixed (I can do unless you get to it before me).

@brosenquist
Copy link

@zbjornson Thanks for the approval. @chearon Fingers crossed this can land very soon!

@zbjornson
Copy link
Collaborator

Not sure when this will get released unfortunately. I started a branch to try to fix the Windows CI issues (caused by GitHub actions updating to VS2022 by default), but then hit an error that's a pain to troubleshoot through CI logs. Once that's resolved, the same fix will have to be made to the prebuild workflow. 😩
https://github.com/Automattic/node-canvas/runs/5342435061?check_suite_focus=true

@brosenquist
Copy link

@zbjornson Thanks again for getting this in. Is the Window CI issue you mentioned related to actions/runner-images#4856 or is there some other issue I should be following to keep on top of when a release might happen? I guess I could just wait for a new tag, but I would prefer to know what progress is being made to resolve the issues blocking the release of this fix.

@zbjornson
Copy link
Collaborator

v2.9.1 is now published.

chearon added a commit that referenced this pull request Jun 24, 2022
the problem was exposed by #1987, but was always there

fixes #2041
zbjornson pushed a commit that referenced this pull request Jun 24, 2022
the problem was exposed by #1987, but was always there

fixes #2041
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.

registerFont does not allow for two or more fonts with the same (preferred) family name, style, and weight
4 participants