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

Update gnome bindings and librsvg #78

Closed
wants to merge 4 commits into from
Closed

Update gnome bindings and librsvg #78

wants to merge 4 commits into from

Conversation

calebcartwright
Copy link
Member

@calebcartwright calebcartwright commented Apr 24, 2022

Best summarized in this comment, but the tl;dr version is that we've been blocked on updating this batch of dependencies for a few months due to one of them (librsvg) increasing the minimum supported version of a system package (pango).

This PR bumps the platform version of our Docker image (and CI) to the newly released Ubuntu 22.04, the latest LTS version. That includes a supported version of pango out of the box (which wasn't readily available in the prior LTS version) which resolved the system package issues referenced in the prior PR.

Will also note taht the update to 22.04 required some minor surgery on the lockfile to account for the upgrade from OpenSSL v1.1.1 to v3.x (a forced update of once_cell which was required to force update native-tls which was required to force update openssl & openssl-sys to versions that can operate with OpenSSL v3.x).

This gets things back in good working order and able to update these dependencies again going forward. This does transitively carry forward some system requirements for us, though in my opinion they are quite reasonable: >= Ubuntu 21.10/Ubuntu 22.04 (LTS), >= Debian 11 (Bullseye), >= Fedora 34, etc., especially since people can always use the Docker process from any environment even if they're on older systems. The minimum supported version of pango has been available on Mac (via homebrew) for quite a long time already, and the Windows story remains centered around WSL where users have all sorts of distro flexibility.

@calebcartwright

This comment was marked as outdated.

@calebcartwright calebcartwright changed the title Awaits with baited breath.... Update gnome bindings and librsvg Apr 24, 2022
@calebcartwright
Copy link
Member Author

calebcartwright commented May 3, 2022

Good shout, I really should've thought to check given the system level nature of the changes. Very much looking forward to getting review apps together and hopefully #35 one day too.

I suspect the change may be deliberate, based on https://developers.googleblog.com/2022/04/what-is-black-and-white-and-read-all.html (probably related to some font priority/selection changes) what are your thoughts?

@chris48s
Copy link
Member

chris48s commented May 3, 2022

Its always emoji 😄

I actually saw that same article the other day.

My assumption is that what we're seeing here is actually not that font. Have a look at the section of the article with the subheading "New designs, fewer details" (sorry - it doesn't have an anchor I can deep-link to) and then compare to:

Aside from that, if possible I think we should try to see if we can find a solution that uses the same emoji set we're working with for now for 2 reasons:

  1. Consistency with SVGs. Your SVG badge is going to look like https://img.shields.io/badge/emoji-%F0%9F%91%8D-blue (although maybe on latest Ubuntu, it doesn't 🤔 ). We should try to make PNG close to that if we can.
  2. A lot of them aren't very clear (at least in badge form). For example if I didn't already know local-emoji-👍-blue is 👍 I probably wouldn't be able to tell.

I'm happy to try and have a look at that when I get a chance.

@calebcartwright
Copy link
Member Author

My assumption is that what we're seeing here is actually not that font

As in, within the execution runtime the noto fonts aren't being used for the emoji? That would surprise me if so, but should be a relatively easy hypothesis to test (will do so later if you don't beat me to it)

Aside from that, if possible I think we should try to see if we can find a solution that uses the same emoji set we're working with for now for 2 reasons

I'm not sure I follow. When you say "set we're using now" what's that in reference to? If it's the svgs then I'm not sure how practical that is going to be given that for svgs that will vary (or at least can vary) from platform to platform and browser to browser. For example on my machine (Ubuntu 22.04 in Firefox) the font used for the emoji in the SVG is Twemoji Mozilla

I'm also not sure if you'd made an inference from my last comment, but to be unequivocally clear, I think this is a problem we need to do something about/I don't think we should proceed as-is with this current state of emoji. However, my first assumption was somehow that the noto emoji fontsets were being used during png creation instead of noto emoji color, unlike what we saw in the prior OS version. If that were to be true, then my instinct would've been to try to correct that. I'm also open to swapping out to a different font pack too, though I think no matter which one we pick there's no chance our pngs will have absolute conformity to svg handling of emoji given the different contexts (assuming that's what you were referring to)

@chris48s
Copy link
Member

chris48s commented May 3, 2022

I'm not sure I follow. When you say "set we're using now" what's that in reference to?

Sorry. I mean we should try to retain the font that squint is using in production today ("noto emoji color"?) e.g:

raster-emoji-👍-blue - https://raster.shields.io/badge/emoji-%F0%9F%91%8D-blue

(if possible).


My assumption is that what we're seeing here is actually not that font

As in, within the execution runtime the noto fonts aren't being used for the emoji? That would surprise me if so, but should be a relatively easy hypothesis to test (will do so later if you don't beat me to it)

I think the font that is being described in https://developers.googleblog.com/2022/04/what-is-black-and-white-and-read-all.html isn't the one we're seeing on the badges generated by the code on this branch. If you compare the flags from the section titled "New designs, fewer details" in that article to

What we're seeing is the exact design decision they're saying they didn't take.

@calebcartwright
Copy link
Member Author

Gotcha, thanks for the clarification! I did a quick spot check in the container to ensure the font package was installed correctly and recognized, but also agree with you that it doesn't seem to be getting used. We probably just need to find the right incantations to chuck into the dockerfile to address that 🤞

@chris48s
Copy link
Member

chris48s commented May 9, 2022

Hmm. I'm stumped on this. Tried various overrides in /etc/fonts/conf.d and /root/.config/fontconfig/conf.d. Tried installing some different emoji fonts (e.g: fonts-emojione) to see if that made any difference.

Rolled back all of that. Fundamentally, Noto Emoji Color is installed. If I run fc-match emoji I get back

NotoColorEmoji.ttf: "Noto Color Emoji" "Regular"

so that should be the font being used.

I think based on that maybe whatever is causing it not to use Noto Color Emoji is the changes at the rust lib layer rather than the changes at the OS layer.

@calebcartwright
Copy link
Member Author

I think based on that maybe whatever is causing it not to use Noto Color Emoji is the changes at the rust lib layer rather than the changes at the OS layer.

I had a similar thought and tried exploring this as well. One of the things I attempted (based on a thread in one of those upstream issues) was to include the noto reference in the font family list within the svg, but that didn't make any difference either 🤷‍♂️

@calebcartwright
Copy link
Member Author

Confirmed that it's related to the lib updates. Updated the OS in the Docker image to the latest, while sticking with the same libs (outside updating to openssl 3) and the emoji are still the same

@calebcartwright
Copy link
Member Author

I'm still not having any luck trying to force the cairo context to use a certain font (the API has such a function, but it doesn't fix the issue). However, I did notice a couple upstream bugs/PRs that were highly font related so probably going to shift back to a wait-with-:crossed_fingers: strategy for the time being

@calebcartwright
Copy link
Member Author

I think it will be easier to start fresh rather than trying to resolve the conflicts. I'll cherry-pick any relevant commits next time I have another go at this

@chris48s
Copy link
Member

If we come back to this, hopefully the image diff tests will make it easier to quickly tell if we've got it right or not 🤞

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.

None yet

2 participants