Parallel execution fontforge in docker #1508
Conversation
There was a problem hiding this comment.
Thanks for the nice PR!
I did never put much effort in the docker patcher ;-) because I do not use it at all.
But than means that suggestions like yours are so valuable!
I think this is a very good improvement. The gotta-patch-em also can parallel-patch (but with a hand-rolled parallelism and a hard wired number of jobs).
I would allow people to specify the -j option, as not all want parallel patching.
Maybe also helpful would be some output if find turned up with nothing.
Tell me if you want to expand this or if you prefer me merging as-is.
bin/scripts/docker-entrypoint.sh
Outdated
| @@ -23,6 +23,6 @@ done | |||
| printf "Running with options:\n%s\n" "$args" | |||
|
|
|||
| # shellcheck disable=SC2086 | |||
| for f in /in/*.otf /in/*.ttf /in/*.woff /in/*.eot /in/*.ttc; do [ -f "$f" ] && fontforge -script /nerd/font-patcher -out /out $args "$f"; done | |||
| find /in -type f \( -name "*.otf" -o -name "*.ttf" -o -name "*.woff" -o -name "*.eot" -o -name "*.ttc" \) | parallel fontforge -script /nerd/font-patcher -out /out $args {} | |||
There was a problem hiding this comment.
I like find much more than the loop. We could use -iname here, as some fonts out there have their name and extension in caps. (Which would be an additional commit of course ;)
There was a problem hiding this comment.
Probably I would use -iregex but that is because I'm a regex person and hate the -o style in find ;-)
There was a problem hiding this comment.
| find /in -type f \( -name "*.otf" -o -name "*.ttf" -o -name "*.woff" -o -name "*.eot" -o -name "*.ttc" \) | parallel fontforge -script /nerd/font-patcher -out /out $args {} | |
| find /in -type f -iregex ".*\.\(otf|ttf|woff|eot|ttc\)" | parallel fontforge -script /nerd/font-patcher -out /out $args {} |
untested
There was a problem hiding this comment.
Dockerfile's find is a BusyBox multi-call, which is not support -iregex, I will change it with -iname.
|
@allcontributors please add @nobk for code |
|
I've put up a pull request to add @nobk! 🎉 |
|
As a side note, I did never check if the debug log will be even avaiable when docker is used. |
I will add an option for |
|
I think docker patcher is for users, not for developers, so I am not test logfile output. |
|
Thank you! Appreciate your work 💚 |
|
Fixing |
$ shellcheck docker-entrypoint.sh
In docker-entrypoint.sh line 28:
-exec fontforge -script /nerd/font-patcher -out /out $args {} \;
^---^ SC2086 (info): Double quote to prevent globbing and word splitting.
Did you mean:
-exec fontforge -script /nerd/font-patcher -out /out "$args" {} \;
In docker-entrypoint.sh line 34:
| parallel $njob fontforge -script /nerd/font-patcher -out /out $args {}
^---^ SC2086 (info): Double quote to prevent globbing and word splitting.
^---^ SC2086 (info): Double quote to prevent globbing and word splitting.
Did you mean:
| parallel "$njob" fontforge -script /nerd/font-patcher -out /out "$args" {} |
|
no, that warnning caused by BusyBox multi-call |
|
Adding one more line of code eliminates those two warnings. |
|
no, thanks :-) |
|
My fix is like this, and not submit that then, it just warning. |
|
You can execute |
Well, that output helps (me) when those users have problems with some particular font. For example
|
|
As you changed the default value of PN to 1, you need modify readme.md docker usage section, default single process, |
|
I didnt intend to change your default, though 😬 Thanks for reporting. I believe |
|
96497b4 |
I am not using -j0 in my PR, I just omitted the -j parameter in shell script by default or PN=0, |
|
omitted the -j parameter means |
But that would require an (additional) if and/or code duplication. In principle we could -parallel --verbose --null "--jobs=${PN}" fontforge -script ../../font-patcher -out ../../out_fonts $args {}
+[-n "$PN"] && jobs=-j${PN}
+parallel --verbose --null "${jobs}" fontforge -script ../../font-patcher -out ../../out_fonts $args {}The problem is that we can not use double qoutes around
Your original PR had the Not having the params double quoted raises That's the reason I used an explicit 100% (i.e. |
|
KISS = keep it simple stupid |
|
Debugging is not about debugging the code but debugging the font. Having code that works (supposedly) but is hard to read does not help anyone in the long run. So looking well IS important. Sorry that you do not like the changes. |
|
The -j0 make max task as many as total font files can not as default, need avoid. |
|
If you want keep the double qoutes, you can write like this jobs="-j 100%"
[[ -n "$PN" ]] && [ "$PN" -gt 0 ] && jobs="-j${PN}" |
I like correct code refactoring, not the introduction of bugs. |
|
That still bugs with an obscure error if someone does
But still you suggest the above code. That will end up as
and that will raise an error because the allowed syntax is only |
Description
Improve the docker execution method, optimize from single process to parallel execution, and increase the speed of font patching.
see issue 1507
Requirements / Checklist
What does this Pull Request (PR) do?
Speed up docker patcher
How should this be manually tested?
Any background context you can provide?
What are the relevant tickets (if any)?
Screenshots (if appropriate or helpful)