Fontbakery: Check 028: Check fails when OFL.txt is present

[eliheuer]

Observed behaviour

Run fontbakery from the root directory of a font repo containing an OFL.txt file and this test will fail.

Expected behaviour

If valid OFL.txt file is in repo root directory, the test should pass.

Resources and exact process needed to replicate

Version: 0.5.2.dev18+gcff658dd

[felipesanches]

works for me.

[felipesanches]

can you please provide further details on how you got it to fail?

[felipesanches]

ok. I executed it from the data/test/mada/ directory and it failed.
We gotta investigate why that's happening. And also add a code-test for that use-case.

[eliheuer]

Ok, I'll pull the latest version. I don't update as much as I would like because I always need to manually update Lib/fontbakery/specifications/general.py to get the MSfontVal stuff working on MacOS.

[eliheuer]

I just pulled from master and now I'm on 0.5.2.dev18+gcff658dd I still get the same FAIL:

:fire: FAIL: Check font has a license.

• com.google.fonts/check/028
• :fire: FAIL No license file was found. Please add an OFL.txt or a LICENSE.txt file. If you are running fontbakery on a Google Fonts upstream repo, which is fine, just make sure there is a temporary license file in the same folder. [code: none]

[felipesanches]

Ok. I got it. Just fixed it :-)

[felipesanches]

I'll send a pull request.

[eliheuer]

I just pulled this and I'm sill getting the same FAIL

[felipesanches]

hmmm... can you provide further details?

Now it is working for me:

[eliheuer]

I'm trying this on: https://github.com/googlefonts/staatliches

[felipesanches]

So, what you mean is that you expect the check to find a license that is not in the same directory as the font files...

[felipesanches]

I think this is dangerous... we may find a license on a folder while checking font files from a different folder. This can lead to false-PASS.

[eliheuer]

The license file should be in the root directory of the git repo. Could we edit the following code so that we keep trying os.chdir('..') until we hit the directory with the .git directory, then check to make sure the OFL.txt file in that directory?

We need a way to find the root directory of the repo of the font we are checking, and then confirm we are in the appropriate root directory. Then we can test the architecture of the repo.

From: Lib/fontbakery/specifications/googlefonts.py

@condition
def licenses(family_directory):
  """Get a list of paths for every license
     file found in a font project."""
  licenses = []
  if family_directory:
    for license in ['OFL.txt', 'LICENSE.txt']:
      license_path = os.path.join(family_directory, license)
      if os.path.exists(license_path):
        licenses.append(license_path)
  return licenses


@condition
def license_path(licenses):
  """Get license path."""
  # return license if there is exactly one license
  return licenses[0] if len(licenses) == 1 else None


@condition
def license(license_path):
  """Get license filename."""
  if license_path:
    return os.path.basename(license_path)


@check(
  id = 'com.google.fonts/check/028'
)
def com_google_fonts_check_028(licenses):
  """Check font has a license."""
  if len(licenses) > 1:
    yield FAIL, Message("multiple",
                        ("More than a single license file found."
                         " Please review."))
  elif not licenses:
    yield FAIL, Message("no-license",
                        ("No license file was found."
                         " Please add an OFL.txt or a LICENSE.txt file."
                         " If you are running fontbakery on a Google Fonts"
                         " upstream repo, which is fine, just make sure"
                         " there is a temporary license file in"
                         " the same folder."))
  else:
    yield PASS, "Found license at '{}'".format(licenses[0])

[eliheuer]

This needs work because it results in a FAIL when one isn't warranted more often than it prevents an unwarranted PASS.

[felipesanches]

I agree that we need some routine to infer where's the root dir.

But simply going deep on .. until a .git is found is not safe since a user might run fontbakery on a directory that is not version-controlled at all. And then the proposed algorithm would end at the system root!

[felipesanches]

Can we start by only accepting the first .. if the current dir is called fonts ?

[eliheuer]

That would be a HUGE improvement. With variable fonts it most likely will be the first .. if the current dir is called fonts.

[chrissimpkins]

You can get the absolute path to the root dir for any repository under git VC with:

git rev-parse --show-toplevel

if that happens to help here.

[felipesanches]

But this check should work with not-versioned font projects as well.

[eliheuer]

Could we just have it check the directory fontbakery is run from for now? I like to run it from the root dir of the fonts repo anyway.

The main problem here is that we shouldn't have to explain to people new to FontBakery that this test is almost always wrong in the markdown output.

[davelab6]

Checking pwd, same path as each font file, if they are not in pwd and
deduplicated, and git rev-parse --show-toplevel (if it returns something)
seems sufficient to me

[m4rc1e]

From an email earlier today.

And some of the test fail cases does not make sense.
eg: Check font has a license , There is a LICENSE.txt in repo with OFL. But test fails.

[davelab6]

There is a LICENSE.txt in repo with OFL. But test fails.

Correct; OFL should be named OFL.txt which is what was recommended by SIL when the OFL was published by them, and has been used consistently since.

I can see that perhaps if Github does not recognize OFL.txt as a valid license filename, it might be good to change all GF projects to require that. But I think the GF API will always regurgitate OFL licenses as OFL.txt, so its a distracting change that has a lot of costs for the GF team if we want to go down that route. And is such a low priority...

[felipesanches]

My next step here before closing this issue will be implementing Dave's proposal of "Checking pwd, same path as each font file, if they are not in pwd and deduplicated, and git rev-parse --show-toplevel (if it returns something)"