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

Docker nerdfonts-patcher errors out on fontforge start #1647

Open
3 tasks done
kjkent opened this issue Jun 1, 2024 · 15 comments · Fixed by #1648
Open
3 tasks done

Docker nerdfonts-patcher errors out on fontforge start #1647

kjkent opened this issue Jun 1, 2024 · 15 comments · Fixed by #1648

Comments

@kjkent
Copy link

kjkent commented Jun 1, 2024

🗹 Requirements

  • I have searched the issues for my issue and found nothing related and/or helpful
  • I have searched the FAQ for help
  • I have searched the Wiki for help

🎯 Subject of the issue

Experienced behavior:

Docker container nerdfonts/patcher errors out when run. I've attempted both compose and run syntax:

 services:
   nerdfonts-patcher:
    command: |
      --mono
      --adjust-line-height
      --progressbars
      --complete
    container_name: nerdfonts-patcher
    image: nerdfonts/patcher
    #user: ${UID}:${GID}
    volumes:
    - ./nf-in:/in
    - ./nf-out:/out

or docker compose run --rm -v ./nf-in:/in -v ./nf-out/out nerdfonts/patcher --mono --adjust-line-height --progressbars --complete.

Both result in:

Running with options:
 --mono --adjust-line-height --progressbars --complete
Parallelism 0
fontforge -script /nerd/font-patcher -out /out --mono --adjust-line-height --progressbars --complete /in/berkeley-mono/OTF/BerkeleyMono-BoldItalic.otf
#### above line repeated for each font

Nerd Fonts Patcher v3.2.1 (4.14.3) (ff 20230101)
Done with Patch Sets, generating font...
Copyright (c) 2000-2023. See AUTHORS for Contributors.
 License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
 with many parts BSD <http://fontforge.org/license.html>. Please read LICENSE.
 Version: 20230101
 Based on sources from 2023-10-26 13:09 UTC-D.
Core python package 'pkg_resources' not found: Cannot discover plugins
Traceback (most recent call last):
  File "/nerd/font-patcher", line 2155, in <module>
    main()
  File "/nerd/font-patcher", line 2147, in main
    patcher.generate(sourceFonts)
  File "/nerd/font-patcher", line 415, in generate
    sourceFont = sourceFonts[0]
                 ~~~~~~~~~~~^^^
IndexError: list index out of range
##### above block repeated multiple times

Expected behavior:

The fonts to patch without the errors
Example symbols:
N/A

🔧 Your Setup

  • Which font are you using (e.g. Anonymice Powerline Nerd Font Complete.ttf)?
    Nerdfonts/patcher docker image
  • Where did you get the file from (download link, self patched, source downloaded from link...)
    Docker Hub
  • Which terminal emulator are you using (e.g. iterm2, urxvt, gnome, konsole)?
    Alacritty
  • Are you using OS X, Linux or Windows? And which specific version or distribution?
    Arch Linux
@kjkent
Copy link
Author

kjkent commented Jun 1, 2024

After some testing out of docker:

  • The errors above happened on variable fonts, and judging from some script output, ie., opening may fail, the results will most likely not be what you want, it appears compatibility with these fonts isn't fully supported.

  • The errors also occurred on .woff and .woff2 files. From the docker entrypoint script, I believed at least .woff would be supported; but, perhaps it's font-specific!

In any case it seems this isn't the container issue I thought, and instead is just incompatibility with variable and potentially web fonts.

If this is expected, I can close this issue? Sorry for the noise if so.

@Finii
Copy link
Collaborator

Finii commented Jun 1, 2024

Thanks for reporting!

Indeed, variable fonts can not be opened (and then patched) with fontforge. This is a fontforge limitation and Nerd Fonts font-patcher needs to be rewritten for some more modern backend to support VF.

But that message "opening may fail" should also be visible in the docker runs 🤔

And this font file name does not sound very variableish /in/berkeley-mono/OTF/BerkeleyMono-BoldItalic.otf.

(On the other hand, I never had the idea to put subdirectories under /in, so that is completely untestet 😬 )

And woffs are also untested, but fontfore should be able to handle them just fine.

Maybe you can give me a link to the fonts that fail and I can investigate further.

The error message with the index 0 out of bounds is completely unexpected.
It seems fontforge.fontsInFile() fails to detect any fonts in the file and does not not error out and WE do not check the result.
I'd fix that, a file that causes this would be great for testing.

@kjkent
Copy link
Author

kjkent commented Jun 1, 2024

Hey, thanks for the feedback! So I think the 'opening may fail' message perhaps was present in the Docker runs, although as the script now runs with parallel, the more noisy errors such as the index issue quickly drown out those lines! I can check :)

The BoldItalic font you mention didn't cause an error -- I truncated some of the logs where text was repeated. When I removed the WOFF/WOFF2/variable fonts, the rest sailed through without any issue (I ran --mono with fonts in subfolders and then without mono after realising I like the bigger glyphs, coincidentally without subfolders, both completed similarly problem-free).

I'd be happy to provide font files, but I'm concerned about running afoul of the licence as it's not a free font. There is a trial download of the font with some vowel glyphs swapped. Let me see if I can find that and what font files are in there. If all else fails I recently spoke with the author of the fontface and think I'll need to contact him again shortly; I can ask for permission if it that's helpful.

I'll also rerun the patcher and more specifically detail what fonts throw the index error, and what similarities they hold.

@Finii
Copy link
Collaborator

Finii commented Jun 2, 2024

Thanks for the additional information!

Maybe it will be more helpful if you run the docker in sequential -j 1 mode, which is a bit harder to achieve. I would have liked the usual j option, but ... sigh. Here the example how to activate it by setting PN (which means process-number, presumably?):

docker run --rm -v /path/to/fonts:/in:Z -v /path/for/output:/out:Z -e "PN=1" nerdfonts/patcher [OPTIONS]

I will fix the empty array / index error. But it would be interesting which font triggers it. I expected it can never turn up empty, and it would be good to understand the situation where my assumptions fail. If you can identify the font file that does, maybe it is also the case with the same file as trial version.

Thanks again for helping 👍

@Finii
Copy link
Collaborator

Finii commented Jun 2, 2024

@allcontributors please add @kjkent for bug

Copy link
Contributor

@Finii

We had trouble processing your request. Please try again later.

@Finii
Copy link
Collaborator

Finii commented Jun 2, 2024

Hmm, it's even documented ;-)

image

Ah, of course it triggers if the specified "font file" is no font at all 🤦‍♀️

image

Finii added a commit that referenced this issue Jun 2, 2024
[why]
When the file specified to be patched is not a font file the patcher run
errors out with an out of index runtime error:

Traceback (most recent call last):
  File "/home/fini/extra/git/nerd-fonts/font-patcher", line 2155, in <module>
    main()
  File "/home/fini/extra/git/nerd-fonts/font-patcher", line 2147, in main
    patcher.generate(sourceFonts)
  File "/home/fini/extra/git/nerd-fonts/font-patcher", line 415, in generate
    sourceFont = sourceFonts[0]
                 ~~~~~~~~~~~^^^
IndexError: list index out of range

[how]
Do not assume that the specified file will be a font file but rather
check if fontforge detects a font in the file and error out if there is
no font found.

Fixes: #1647

Reported-by: Kristopher James Kent <[email protected]>
Signed-off-by: Fini Jastrow <[email protected]>
@Finii
Copy link
Collaborator

Finii commented Jun 2, 2024

Ok, thanks again, this will be fixed by the PR (see above).

Now I need to find out what's wrong with the allcontributors bot 😬

Finii added a commit that referenced this issue Jun 2, 2024
[why]
When the file specified to be patched is not a font file the patcher run
errors out with an out of index runtime error:

Traceback (most recent call last):
  File "/home/fini/extra/git/nerd-fonts/font-patcher", line 2155, in <module>
    main()
  File "/home/fini/extra/git/nerd-fonts/font-patcher", line 2147, in main
    patcher.generate(sourceFonts)
  File "/home/fini/extra/git/nerd-fonts/font-patcher", line 415, in generate
    sourceFont = sourceFonts[0]
                 ~~~~~~~~~~~^^^
IndexError: list index out of range

[how]
Do not assume that the specified file will be a font file but rather
check if fontforge detects a font in the file and error out if there is
no font found.

Fixes: #1647

Reported-by: Kristopher James Kent <[email protected]>
Signed-off-by: Fini Jastrow <[email protected]>
@Finii Finii closed this as completed in b95c671 Jun 2, 2024
@Finii
Copy link
Collaborator

Finii commented Jun 2, 2024

@allcontributors please add @kjkent for bug

Copy link
Contributor

@Finii

I've put up a pull request to add @kjkent! 🎉

@kjkent
Copy link
Author

kjkent commented Jun 15, 2024

Hey @Finii, I've been digging into this a little bit more. It appears to be an upstream issue with fontforge, as in, fontsInFile() is currently incompatible with woff fonts: fontforge/fontforge#1964.

As a workaround, those with the woff2 library installed can run woff2_decompress prior to running the script, then woff2_compress after. The files I tested underwent this process without issue. It's woff2-specific though (sorry woff1)

If you'd find it beneficial, I could make a PR handling this in the script, either checking whether the user has https://github.com/google/woff2 installed and using it from PATH, or, as a fallback, cloning and building it. Admittedly, it's a bit of a hack workaround, so I understand if you'd rather leave it to the user :D

@kjkent
Copy link
Author

kjkent commented Jun 15, 2024

Oh! And if it's useful to anyone reading this, I figured a way for users to grab the latest font-patcher and its deps without pulling the kitchen sink with it. It was useful for me for scripting as I was experimenting with the NerdFontsSymbolsOnly TTF. I found sparse-clone and cone mode very fickle to have it behave how I'd expect it, but the following does the job:

repo='https://github.com/ryanoasis/nerd-fonts'
branch='master'

# Accepts directories and single files
git_include=(
        '/font-patcher'
        '/src/glyphs/**'
        '/bin/scripts/name_parser/**'
)

git clone -n --depth=1 --filter=tree:0 "$repo" "${repo##*/}"
cd "$_"
git sparse-checkout set --no-cone ${git_include[@]}
git checkout "$branch"

@Finii
Copy link
Collaborator

Finii commented Jun 16, 2024

Thanks for sharing the interesting find above regarding woff2!

I need to digest and let that simmer a bit ;-) But I want to comment on this right away:

Oh! And if it's useful to anyone reading this, I figured a way for users to grab the latest font-patcher and its deps without pulling the kitchen sink with it.

Why not just using the FontPatcher.zip; I believe that is far easier to grasp for usual users that have a very specific sparse checkout. Well, except you want to contribute.

https://github.com/ryanoasis/nerd-fonts/blob/master/FontPatcher.zip is updated automagically (via CI) whenever anything related to selfpatching changes in the default branch.

@kjkent
Copy link
Author

kjkent commented Jun 20, 2024

@Finii oh yeah, no, you're absolutely right. What I wrote is probably more specific to my use case than others, I'm writing a python script to generate web fonts and strip unused glyphs out of the resultant file. My initial (convoluted) attempt used the icons-only font file as a reference, so, the sparse clone came in handy.

I think I was in too deep and didn't realise that it was very much a me problem haha.

@Finii
Copy link
Collaborator

Finii commented Jun 20, 2024

After having read your report at the Fontforge repo I believe my fix could in fact be wrong (too limiting).
Reopening to investigate.
Thanks!

@Finii Finii reopened this Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants