fonttools / fontbakery

🧁 A font quality assurance tool for everyone
https://fontbakery.readthedocs.io
Apache License 2.0
558 stars 103 forks source link

"No applicable files found" when font file is named (like) "A[aa]a.ttf" #4005

Closed justvanrossum closed 1 year ago

justvanrossum commented 1 year ago

Describe the issue

(Note the "a" after the "]")

$ fontbakery check-universal A\[aa\]a.ttf 
No applicable files found
usage: fontbakery check-universal [-h] [--configuration CONFIGFILE] [-c CHECKID]
                                  [-x EXCLUDE_CHECKID] [-v] [-l LOGLEVEL]
                                  [-m LOGLEVEL_MESSAGES] [--succinct] [-n] [-C] [-S]
                                  [-L] [-F] [--dark-theme] [--light-theme]
                                  [--timeout TIMEOUT] [--json JSON_FILE]
                                  [--badges DIRECTORY] [--ghmarkdown MD_FILE]
                                  [--html HTML_FILE] [-g ITERATED_ARG] [-o ORDER]
                                  [-J JOBS] [-j]
                                  [files ...]
$ ls -l A\[aa\]a.ttf 
-rw-r--r--@ 1 just  staff  463580 Jan  9 13:09 A[aa]a.ttf

Additional context

This is with fontbakery==0.8.11a8

The actual contents of the file doesn't seem to affect this issue (I tried with two different valid fonts).

Cc: @BlackFoundry

felipesanches commented 1 year ago

What is the stuff you place between the square brackets? Are those VF axis tags?

If so, would it be OK for fontbakery to simply ignore any extra characters placed after the brackets, still treating the filename as indication that this is a variable font?

I am also curious to learn what's the purpose of those extra characters in your fontfile naming scheme.

justvanrossum commented 1 year ago

What is the stuff you place between the square brackets? Are those VF axis tags?

We do yes, but the contents of that part does not affect the problem. That's why I reduced it to this simple example. Which is enough to make it reject the font.

If so, would it be OK for fontbakery to simply ignore any extra characters placed after the brackets, still treating the filename as indication that this is a variable font?

Why would it treat the filename as an indication that it is or is not a variable font? The presence of fvar should determine that, nothing else. Checking whether the filename conforms to one naming scheme or other is fine and useful, but if fontbakery is drawing conclusions based on the filename alone, that seems misguided to me.

I am also curious to learn what's the purpose of those extra characters in your fontfile naming scheme.

Why does it matter? This is a valid font with a file name, and fontbakery gives a confusing error message and rejects the font outright. It's fine if fontbakery reports a FAIL on this filename, but it should be no reason to not process the font.

felipesanches commented 1 year ago

Why would it treat the filename as an indication that it is or is not a variable font? [...] if fontbakery is drawing conclusions based on the filename alone, that seems misguided to me.

Well... we do infer familyname, stylename and eventual VF axes by inspecting the filename, so that we can later lookup these same things inside the font binary and make sure there's no mismatch between filename and actual binary content.

Which is the source of truth of a given aspect of a font is sometimes difficult to decide, especially in cases for which there are multiple table entries for the same kind of information. (but that's not the case for varfonts, as the mere presence of fvar, as you mentioned, is unambiguous indication of a font being variable)

This issue highlights the fact that fontbakery currently enforces a certain file naming scheme defined by Google Fonts, and I think there should be a way to use the tool without this Google Fonts bias. Any font filename should be accepted. But then, the challenge is that we'd need to map out how much of the current set of checks depends on inferring things from the filename.

Also it would be useful to better understand what are the other file naming schemes people have been using. That's why I was curious about your scheme. That's why it matters - for us to better grasp the problem.

Note: This is just a bit of quick brainstorming on the subject, but... would it be a solution for this if we placed filename parsing inside vendor specific profiles? Yet, how to handle this for the universal profile? (end of quick thought)

I acknowledge that fontbakery is currently strongly opinionated about filenames, but I do not want to put a lot of effort into changing that behavior before I have lots of evidence of a real problem so that a good plan for changes can be put in place.

justvanrossum commented 1 year ago

we do infer familyname, stylename and eventual VF axes by inspecting the filename, so that we can later lookup these same things inside the font binary and make sure there's no mismatch between filename and actual binary content

But this should be local to a check, and should work the other way around: "given this and that family name, style name, VF axes etc. as seen in the font data, we expect the filename to be XXX". The font must always be the source of truth, and not the filename.

would it be a solution for this if we placed filename parsing inside vendor specific profiles?

No.

I don't mind that fontbakery is opinionated about filenames in specific checks (and I often appreciate it), but fontbakery should work on any font regardless of the filename. That's just a very fundamental issue. I seriously don't understand how filename parsing can be seen as something bigger than "a check".

justvanrossum commented 1 year ago

It's as if fontbakery decides to not process a font because it doesn't contain a glyph for "a".

justvanrossum commented 1 year ago

Are we perhaps having a misunderstanding? To be ultra clear: a font file named A[aa]a.ttf is not checked at all. It's not one check that is failing, it is all of fontbakery that fails to recognize the font file.

anthrotype commented 1 year ago

The font must always be the source of truth, and not the filename.

big 👍 on that - incredible that this even needs to be spelled out

yanone commented 1 year ago

Btw, just today I had fontbakery also not process a file called ShantellSans-Italic[BNCE,INFM,SPAC,wght].converted.ttf. The .converted was added by a script that I used. It was indeed checked when pointing fontbakery to *.ttf, but only while other fonts were indeed present in the same folder. As soon as that file existed on its own, even *.ttf wouldn't find the file.

That's probably a good bit of information for a first step of fixing this without changing the entire source of truth around.

felipesanches commented 1 year ago

it is all of fontbakery that fails to recognize the font file.

Yes, I understood that, and I agree it is a serious bug that needs fixing.

I am just taking the opportunity to discuss the big picture instead of making a quick dirty fix with no deeper reasoning on the best way of doing things

justvanrossum commented 1 year ago

with no deeper reasoning on the best way of doing things

This reasoning should be plenty: "fontbakery should not reject a font file without any checking based on the filename".

yanone commented 1 year ago

Also, we're dealing with statics, too, not just VFs. When checking for whether or not the Bold bit is set in fsSelection, for example, I don't see how else the truth can be sourced other than from the file name.

It's great that we're talking about this today, as I'm just now reworking the style condition of fontbakery’s GF profile that has so far only returned useful values for statics and now we're retouching it for VFs.

anthrotype commented 1 year ago

the filename is supposed to reflect the content of the font, not the other way around. And fontbakery must not infer anything nor outright reject checking a font based on the filename alone. Filename is just one among several things to be checked.

davelab6 commented 1 year ago

The font must always be the source of truth, and not the filename.

big 👍 on that - incredible that this even needs to be spelled out

No, its been gf policy for over a decade that the filename is SOT for gf profile: That's what users see when managing files, not internal metadata, and it's trivial to edit to correct.

felipesanches commented 1 year ago

@justvanrossum, what is the meaning of the extra chars after brackets on your file naming scheme?

In case the information is not confidential, could you please let me know the real file name instead of A[aa]a.ttf? I'm legitimately curious to see what is appended to the filename in your case.

justvanrossum commented 1 year ago

@justvanrossum, what is the meaning of the extra chars after brackets on your file naming scheme?

None whatsoever. It's similar to what Yanone just commented. Again: it. does. not. matter.

felipesanches commented 1 year ago

ok. Similar to Yanone's case. Thanks for the answer.

justvanrossum commented 1 year ago

The bug is here: https://github.com/googlefonts/fontbakery/blob/c7c546ef0f5cdd350b537b425459ace221f7a829/Lib/fontbakery/fonts_profile.py#L58-L62

felipesanches commented 1 year ago

I was planning to fix it after lunch (I am actually cooking right now, :-D). But if you're already with your hands at the code, a PR would also be appreciated if you feel like doing it. I can review your PR later.

BlackFoundry commented 1 year ago

The font must always be the source of truth, and not the filename.

big 👍 on that - incredible that this even needs to be spelled out

No, its been gf policy for over a decade that the filename is SOT for gf profile: That's what users see when managing files, not internal metadata, and it's trivial to edit to correct.

If it’s some file name structure is mandatory, the error message shoudl at least be clearer than "No applicable files were found"; we thought the binary was corrupted somehow… But I think Just and Cosimo are right, file name should be just a check, and not block the entire checking process.

felipesanches commented 1 year ago

Yes, they're right. I agree. No doubt about this. We'll fix asap.

felipesanches commented 1 year ago

thanks, @justvanrossum