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

Fix logging error font-patcher #1287

Merged
merged 2 commits into from
Jun 6, 2023
Merged

Fix logging error font-patcher #1287

merged 2 commits into from
Jun 6, 2023

Conversation

llc0930
Copy link
Contributor

@llc0930 llc0930 commented Jun 5, 2023

Description

Python complains about error on line 1881 in font-patcher.
Maybe you guys should take the time to check if the function global logger is doing exactly the right thing.

Requirements / Checklist

What does this Pull Request (PR) do?

Fix typo in font-patcher.

How should this be manually tested?

I'm not sure, maybe this feature isn't finished yet?

Any background context you can provide?

Python complains about error when I manually patch fonts.
The shell script I use is:

for p in ../iosevka-custom-mono/ttf/*.ttf; do
	echo -e "Patching for font \e[0;36m${p:27}\e[0m ..."
	fontforge -script font-patcher "$p" -cq --careful -out iosevka-custom-mono-nerd --makegroups 0
done

When I do not set the parameter --makegroups 0, it reports an error.

@Finii Finii added the confirmed label Jun 6, 2023
@Finii Finii changed the title Fix typo in font-patcher. Fix logging error font-patcher Jun 6, 2023
@Finii
Copy link
Collaborator

Finii commented Jun 6, 2023

Thanks for finding and fixing this!

There are two more affected calls in the code. To prevent that I added an additional commit that simplifies the whole logger business and allows the programmer to use logger throughout and not decide on a case-to-case basis if logger or logging is the correct thing to use.

Maybe you guys should take the time to check if the function

Yes. That is often skipped due to laziness.
On the other hand that logging message you stumbled over is just issued if some user did not follow the instructions and is kind of a saveguard for 'shall never happen'. Misuse cases are tested less rigorously, I have to admit.

P.S. I guess you should use --makegroups 4, but of course I do not know your font files. At least the 'original' Iosevka needes that.

[why]
Usually the variable `logger` holds the logger object and all logging
calls got through that.

But because we use the font filename as loggername that logger object
can only be set up after the arguments have been parsed. If some
messages are to be logged before the call needs to go to the root logger
called as `logging` class.

This means one needs to take `logger` or `logging` based on the time
when someting is to be logged. That can be confusing and is easily
wrong, especially if code is shifted.

[how]
Always use the `logger` variable and just let that point to the root
logger until we set up a concrete logger.

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

Finii commented Jun 6, 2023

Add skip-ci, force push

@Finii Finii merged commit df626d3 into ryanoasis:master Jun 6, 2023
@llc0930
Copy link
Contributor Author

llc0930 commented Jun 6, 2023

In fact in older versions I used to just use the command:
fontforge -script font-patcher "$p" -q -c --careful -out iosevka-custom-mono-nerd

But when I did the long-lost update yesterday, I found that there would be a warning, and it is still the same now.
2023-06-06-23:42:48-w
So why doesn't the default --makegroups 1 work?

Thanks for the suggestion, but --makegroups 4 doesn't work either.
2023-06-07-00:28:03-w

@llc0930
Copy link
Contributor Author

llc0930 commented Jun 6, 2023

I use Debian sid, here is the script file I use if it helps.
iosevka_build.sh.txt
func.sh.txt

@Finii
Copy link
Collaborator

Finii commented Jun 6, 2023

How did you obtain the font-patcher?

https://github.com/ryanoasis/nerd-fonts#font-patcher

image

Edit:

--makegroups 0 is a last resort when the FontnameParser can not be loaded. It handles all the font name related stuff.
The resulting fonts might work, but most times not if there are multiple weights in a font set.

@llc0930
Copy link
Contributor Author

llc0930 commented Jun 6, 2023

Do I need python3-argparse-addons?
I think I have all other dependencies.
2023-06-07-01:56:28-r

@Finii
Copy link
Collaborator

Finii commented Jun 6, 2023

I believe you dont. Just try to run the patcher, it should tell you missing dependencies.

@llc0930
Copy link
Contributor Author

llc0930 commented Jun 6, 2023

After installing python3-argparse-addons, the same error is reported without the parameter --makegroups, and --makegroups 4 also does not work.
Ok, I'll try running the patcher directly.

@Finii
Copy link
Collaborator

Finii commented Jun 6, 2023

Same error, meaning FontnameParser missing?

How did you obtain the font-patcher?

@llc0930
Copy link
Contributor Author

llc0930 commented Jun 6, 2023

I figured out what was wrong, I didn't need python3-argparse-addons, my git sparse-checkout command in shell script was wrong.
Instead of git sparse-checkout set "src" I should use git sparse-checkout set --no-cone bin/scripts/name_parser src/glyphs /font-patcher.
There should be no problem now, I will look at the difference between --makegroups 1 and --makegroups 4, thank you.

@Finii
Copy link
Collaborator

Finii commented Jun 6, 2023

As written in the readme, reccommended is using the FontPatcher.zip or the docker image, and not a (sparse) checked out repo.

But now it does not matter anymore ;) 👍

@Finii
Copy link
Collaborator

Finii commented Jun 7, 2023

difference between --makegroups 1 and --makegroups 4

It's not very well documented.

1 has all name parts in full (no abbreviations). Often the resulting name are longer than some old systems can work with. This is the best for modern systems but can have problems on some.

2 abbreviates the the style and weight parts a bit, like "ExtraBold" becomes "ExtBld".

3 abbreviates them even more aggressive, so "ExtraBold" becomes "XBd" or something.

The options 4 to 6 are similar, but tackle additionally the problem when not the styles and weights are the problem but the plain family name. Instead of adding "Nerd Font Mono" or similar to the family name it adds the abbreviated form "NFM". This is needed where the original name is already very long.

5 and 6 behave like 2 and 3, but with NFM etc.

Option 0 produces names with a much simpler scheme, which breaks often.

Option -1 tells the patcher to leave the names alone and not touch them. This can be used for example when patching over an already Nerd Fonts patched font (yes, people do that) or similar.

@llc0930
Copy link
Contributor Author

llc0930 commented Jun 7, 2023

Thanks for the detailed explanation, I ended up going with option -1 yesterday because the fonts I patched are only used on my own system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants