-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Modernize font creation #624
Conversation
* Automate the build process, instead of writing a complicated manual * Update sources used by the docker image away from google code * Exclude MathJax-dev checkout from docker image, to allow image reuse * Try to keep the individual image layers small by combining RUN commands and removing temporary files used only during each individual step
@gagern cool. Is there a way to supply a local path for MathJax-dev? If not, pushing to a remote branch is fine since people will have to do that anyways to create a PR. |
I was wondering about the best approach for local repos, too. Theoretically it should be possible to run a git server locally, and have the docker connect to that. But seeing all the trouble the screenshotter had to go to in order to identify an ip address to be used for this, I'd rather avoid that. Perhaps we should drop the git checkout altogether, and instead download the tarball, or offer a way to copy a local tarball into the container instead. Then we can have the outer script create such a tarball in case the argument is a local directory, or use it as is if it is a file, or download it if it is a url. |
Also use hash of Dockerfile to compute image tag, in order to ensure creation of a new image if the Dockerfile changes.
Another option might be to install whatever dependencies MathJax-dev needs to run the |
I'll leave that to you. Personally I think having both the docker approach and a non-dockerized version would be nice. So I'd say merge this and if one of us finds the time we can write a Makefile to build all dependencies and use that. |
RUN wget "https://archive.apache.org/dist/xmlgraphics/batik/batik-1.7.zip" \ | ||
&& unzip batik-*.zip batik-*/batik-ttf2svg.jar \ | ||
&& mv batik-*/batik-ttf2svg.jar /usr/share/java/ \ | ||
&& rm -r batik-* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about the benefit of combining multiple commands vs. running them separate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each RUN
command creates a separate layer of docker file system, while commands combined into one RUN
will just contribute one layer that contains their combined effect. In this case, the resulting layer should only contain /usr/share/java/batik-ttf2svg.jar
, as everything else is created and removed in the same RUN
. This keeps the number of layers down and the total size of the image small, in order to conserve resources.
&& sed -i "1s/^/#include <cstddef>/" ttf2eot-*/OpenTypeUtilities.h \ | ||
&& make -C ttf2eot-*/ \ | ||
&& mv ttf2eot-*/ttf2eot /usr/bin/ \ | ||
&& rm -r ttf2eot* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rm
commands are necessary are they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary for the image to perform as intended, no. The goal is again to clean up all the build-time resources once the desired command has been installed, to keep the size of the layer down.
RUN cp MathJax-dev/default.cfg MathJax-dev/custom.cfg | ||
RUN make -C MathJax-dev custom.cfg.pl | ||
|
||
RUN wget "https://github.com/google/woff2/archive/d9a74803fa884559879e3205cfe6f257a2d85519.tar.gz" -O woff2.tar.gz \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
popd | ||
elif [[ -f "$1" ]]; then | ||
FILE="$1" | ||
elif [[ "$1" = http?(s)://* ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried running ./buildFonts.sh /local/path/to/MathJax-dev
and got the following error:
./buildFonts.sh: line 97: syntax error in conditional expression: unexpected token `('
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please try whether a shopt -s extglob
at the beginning of the script fixes this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave that a try, but that gives the same output. :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we can make that just cover http for now, or use [[ "$1" = http://* ]] || [[ "$1" == https://* ]]
if we can't get this glob syntax to work as intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry... I pasted it in the wrong .sh file. It's running now. I'll post back once it completes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It worked! All of the font files were modified. Some of those changes might be due to version changes in the deps and/or running in the docker vs. OS/X itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check whether running the tool on my system will modify them again or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was running it against HEAD
on master
of khan/MathJax-dev
which has a number commits since the last time the font files were generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget whether I ran the screenshot tests after all the font files were changed. I want to get this merged so I'm going to run the script again and then run the screenshot tests and if everything passes I'll merge this.
tar cf /fonts.tar ${filetypes[*]/#/fonts/OTF/TeX/}" | ||
|
||
echo "Creating and starting docker container from image $IMAGE" | ||
CONTAINER=$(docker create "$IMAGE" /bin/sh -c "${CMDS}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the docker image created by the DockerFile is a base image that you then modify to run the commands to actually create the fonts, is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not exactly. Docker uses an image as a template to create containers. So in our case the image contains all the tools we need to create a font, while the container then runs the named commands to actually perform the font creation. You can use the same image to build several fonts, instantiating one container each time you do so. The commands are executed once inside the container constructed here, but the file system that results from running these commands is not preserved, contrary to how images work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A "container" seems like an "instance" from the description you gave. This is probably one of the reasons why I've had trouble grokking docker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A container is an instance, yes. Container is to image like process is to executable file.
Just looking at all the deps, it's going to be a pain to create a cross platform script to install all the deps. I think I installed all the deps manually the last time I edited a font and it was a pain. This will be a huge improvement. Thanks for making it work with local directories. |
I added To test this I checked out Khan/MathJax-dev@677fd6a and ran
This resulted in all font files being updated. To verify that it was only the timestamps being updated I made sure I had a locally copy of https://github.com/Khan/KaTeX/blob/master/.gitattributes and followed its instructions modifying ~/.gitconfig appropriately. Then I ran
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gagern thanks for modernizing our font creating. I really like the ability to point this script at different folders. In the past I made changes to https://github.com/Khan/MathJax-dev from inside the docker instance which worked, but was cumbersome. This is much easier. Sorry for the delay in getting this reviewed.
Motivated by #614 (comment).