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

more font packs in docker image #61

Merged
merged 1 commit into from
Feb 21, 2022
Merged

more font packs in docker image #61

merged 1 commit into from
Feb 21, 2022

Conversation

calebcartwright
Copy link
Member

Resolves #60

@shields-cd shields-cd temporarily deployed to squint-pr-61 February 20, 2022 20:08 Inactive
Dockerfile Outdated
@@ -40,7 +40,8 @@ ENV SERVER_BINARY_NAME=${SERVER_BINARY_NAME}
RUN echo "ttf-mscorefonts-installer msttcorefonts/accepted-mscorefonts-eula select true" | debconf-set-selections
RUN echo 'debconf debconf/frontend select Noninteractive' | debconf-set-selections
RUN apt update -y && apt install -y libgtk-3-dev ttf-mscorefonts-installer
RUN apt install -y fonts-noto-color-emoji
RUN apt install -y fonts-noto-color-emoji fonts-noto-cjk
Copy link
Member Author

Choose a reason for hiding this comment

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

cjk covers a fairly large portion of the unicode block too so figured I'd check to see if that would cover us on the emoji front; it doesn't, so both packages are still needed

Copy link
Member

Choose a reason for hiding this comment

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

I was having a look at this :)

It seems like there are a whole bunch of these noto font packs in Ubuntu:
https://packages.ubuntu.com/search?keywords=fonts-noto
but I'm not exactly sure what is/isn't in each one.

Do you reckon its worth just installing the whole lot so we don't just end up whack-a-mole-ing font packs one at a time as people try different characters?

Copy link
Member Author

@calebcartwright calebcartwright Feb 20, 2022

Choose a reason for hiding this comment

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

It's a good question, and one I've gone back and forth on. The one thing that gives me pause is the risk (however slight) of undesirable impacts from extra packs being present. I'm not 100% convinced the rasterization internals in our dep tree will strictly honor the priority list included in our svg, so unsure whether the presence of additional fonts could result in a wrong/inferior one getting used in a way that might be difficult to hunt down.

Upon additional reflection I think it's probably safe to do so with just the noto fonts, but I'm not sure I'd want to go full blown equivalent with Ubuntu desktop flavors just yet.

Happy to close this in favor of what you're working on if you've got that wired up already

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I run Ubuntu as my desktop OS for developing shields (and have done for many years). I've got all of these packages installed have no such issues. I suspect even if there are any of those issues, its not like its going to produce anything that looks worse than

v1

which is what's happening in production now. I tried building the docker image with:

RUN apt install -y fonts-noto

(which installs all of those font packs). Did a bit of testing. Everything looks right to me, so I suggest we go with that.

Copy link
Member Author

@calebcartwright calebcartwright Feb 21, 2022

Choose a reason for hiding this comment

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

FWIW, I run Ubuntu as my desktop OS for developing shields (and have done for many years). I've got all of these packages installed have no such issues.

I do as well.

The piece that I think is slightly different is that Shields development hasn't dealt with librsvg/cairo rasterization interactions before (at least afaik), nor do I think that's a common interaction in the standard utilization of the desktop environment (including with gnome).

I forget the specifics, but in one of the deep dark rabbit holes I found myself in during the initial development and FTB mess, I was seeing something unexpected on the font that cairo was selecting/somehow being instructed to take. That was also when I was strictly running the native binary directly on my machine, not dealing with docker containerization. It was to the extent that I nearly went down the path of using some other, more complex portions of cairo's API surface that allowed for more explicit font specifications.

Fortunately, that ended up being obviated by the overhaul we made to the svgs upstream in Shields itself, but it definitely left me with a sense of caution/hesitancy. Again I don't have any concerns with grabbing all the noto fonts and have updated things accordingly. I do, however, still have some reservations about adding a bunch of packs preemptively such that an fc-list invocation in the container image produces the same set as we'd see locally.

Fully aware that means there will almost certainly be some similar cases in the future (there's plenty not covered by noto), but I'm definitely still in the adding-as-needed camp at this point

Copy link
Member

Choose a reason for hiding this comment

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

Not suggesting we do it now, but one thing that might be a really good fit for this specific project to spot any changes like this if and when we add more font packs might be to build the docker image then black box test some outputs with something like pixelmatch to make sure the rendered outputs for known good badges don't change.

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds like a good idea! With the old raster proxy we had a test that ensured the response was indeed in a raster format but that was about it, and pixelmatch definitely seems like something worth investigating

@calebcartwright calebcartwright merged commit 8a6c9bb into main Feb 21, 2022
@calebcartwright calebcartwright deleted the more-fonts branch February 21, 2022 21:04
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.

PNG badge creation is broken in non-ASCII characters
3 participants