fontbakery: init at 0.10.40 #228099
fontbakery: init at 0.10.40 #228099
Conversation
|
Result of 85 packages built:
|
There was a problem hiding this comment.
Great PR overall! General tip: Because there are so many new packages, it'd be nice to test them on Darwin with ofborg as well. You could trigger that automatically if all the commits' messages' titles will be change from python3Packages... to python311Packages.... Only the latest is directly evaluated by Hydra, and ofborg counts on that as well.
Also, sorry for waiting so long for a review. We'll sure get all of these packages as quick as possible.
P.S There's a small, trivial merge conflict.
| @@ -13,13 +14,26 @@ buildPythonPackage rec { | |||
| hash = "sha256-fdI4CBUSMbdKW0qg4y22wjWI6bhotgDkQdFMc9X00as="; | |||
| }; | |||
|
|
|||
| patches = [ | |||
| ./0001-use-packaged-unicode-data.patch | |||
There was a problem hiding this comment.
- Add a comment near the patch, and squash this commit to the
- Instead of the
substituteInPlaceused inpreBuild, use the Nix functionsubstituteAll. - Squash this commit to the previous,
initcommit.
|
|
||
| # Package has no unit tests, also it requires network access so we can't | ||
| # try to exercise it in checkPhase. | ||
| pythonImportsCheck = [ "youseedee" ]; |
There was a problem hiding this comment.
Same here, doCheck = false won't hurt.
| # https://github.com/simoncozens/fontFeatures/pull/60#issuecomment-1518963962 | ||
| # Have to write out the -k option instead of using disabledTests because | ||
| # of the test parameter. | ||
| "-k 'not (test_round_trip and mark_attachment)'" |
There was a problem hiding this comment.
Is it possible to use disabledTests here instead?
There was a problem hiding this comment.
No, that didn't work because it's a parametrized test... but the test is passing in the latest upstream release now.
| pytestCheckHook | ||
| uharfbuzz | ||
| youseedee | ||
| ]; | ||
|
|
||
| pythonRelaxDeps = [ "protobuf" ]; |
pkgs/top-level/all-packages.nix
Outdated
| @@ -1508,6 +1508,8 @@ with pkgs; | |||
|
|
|||
| font-v = with python3Packages; toPythonApplication font-v; | |||
|
|
|||
| fontbakery = with python3Packages; toPythonApplication fontbakery; | |||
There was a problem hiding this comment.
Is this package supposed to be available as a python module as well?
There was a problem hiding this comment.
It required as a module for https://github.com/notofonts/notobuilder/ which we would need if we ever want to build noto-fonts from source
There was a problem hiding this comment.
All the packages in this PR should be available as Python modules (since a lot of font build processes will depend on them that way, even if they're really a command line tool like fontbakery). Plus for all those packages where upstream's docs give some indication that the tool is designed to be invoked from the command line, I've exported them as Python applications as well.
| ]; | ||
|
|
||
| postInstall = '' | ||
| install -D -m0644 snippets/fontbakery.bash-completion \ |
There was a problem hiding this comment.
Nit: It might be safer to use installShellFiles.
| # Fix tests to work with ufo2ft >= 2.29.0 | ||
| # https://github.com/googlefonts/fontbakery/pull/4126 | ||
| ./0001-fix-tests-to-work-with-ufo2ft-2.29.0.patch | ||
| # Fix tests when not inside a fontbakery git checkout | ||
| # https://github.com/googlefonts/fontbakery/pull/4125 | ||
| ./0002-don-t-assume-the-tests-are-always-running-in-a-git-c.patch | ||
| # Mock HTTP requests in tests (note we still have to skip some below) | ||
| # https://github.com/googlefonts/fontbakery/pull/4124 | ||
| ./0003-use-not-listed_on_gfonts_api-condition-instead-of-no.patch | ||
| ./0004-use-requests-instead-of-download_file-in-github_gfon.patch | ||
| ./0005-mock-HTTP-requests-in-tests.patch |
There was a problem hiding this comment.
Tests that have open PRs, please use fetchpatch. I guess many of these patches are already merged upstream so hopefully this will look different after you'd revisit this PR.
|
Thanks for the review, and for the tip about I'll rebase and update this PR in the next few days when I get some spare time. |
|
I've rebased this PR, fixed the conflicts, and amended it as follows:
|
|
Result of 89 packages built:
|
|
Sorry for not responding to this. I haven't forgotten this PR. Hopefully will find time to approve it tomorrow or next week. |
| unzip | ||
| ]; | ||
| preCheck = '' | ||
| unzip -o $dist/*.whl |
There was a problem hiding this comment.
Hmm interesting. Could you explain this a bit in a comment?
There was a problem hiding this comment.
This is a bit of a dodgy hack and I shouldn't have left it in (not without a comment).
The actual problem is the classic pytest import confusion. Because of pytest's manipulations of sys.path, the test process ends up importing kurbopy/__init__.py from the source directory, which does not contain the built .so, and so it fails like this:
/nix/store/qp5zys77biz7imbk6yy85q5pdv7qk84j-python3-3.11.6/lib/python3.11/importlib/__init__.py:126: in import_module
return _bootstrap._gcd_import(name[level:], package, level)
tests/test_basic.py:1: in <module>
from kurbopy import Point, Vec2, TranslateScale
kurbopy/__init__.py:1: in <module>
from .kurbopy import Point as Point
E ModuleNotFoundError: No module named 'kurbopy.kurbopy'
We want it to import kurbopy from the nix store via PYTHONPATH instead.
So this hack was stealing $dist and using it to populate the source directory with a usable copy of the built .so, but a proper solution would be to make pytest import kurbopy from the installed directory like a proper installCheckPhase is supposed to do.
Pytest has an --import-mode option which should help with that kind of mess. But no matter what I do, sys.path always imports from . first. I think it's because pytestCheckHook does python -m pytest, and that's one of the side effects noted in the docs.
Anyway, I can just hack it to delete the module from the source directory in preCheck, so that it imports the installed copy from the nix store instead. With a comment explaining what's going on. :-)
There was a problem hiding this comment.
Anyway, I can just hack it to delete the module from the source directory in
preCheck, so that it imports the installed copy from the nix store instead. With a comment explaining what's going on. :-)
Thanks for the thorough explanation @danc86 . Could you do me please 1 more favor? In the very well written comment, could you mention that this is a common issue in nixpkgs and it is investigated in #255262 ?
There was a problem hiding this comment.
The PR should be ready after issue #255262 is mentioned.
There was a problem hiding this comment.
Sure, I've reworded the comment to mention #255262.
|
Also, there's a merge conflict, now I'm noticing - probably fixable easily. |
|
I've rebased this and amended it as follows:
|
|
Building |
|
Thanks for the reviews. :-) |
Description of changes
Font Bakery is a command-line tool for checking the quality of font projects.
It's used for example in the build process for the Inter font family.
This PR also includes a number of new Python packages which are dependencies of fontbakery.
Things done
[ ] For non-Linux: Issandbox = trueset innix.conf? (See Nix manual)NixOS test(s) (look inside nixos/tests)or, for functions and "core" functionality, tests in lib/tests or pkgs/testmade sure NixOS tests are linked to the relevant packagesnix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)[ ] (Package updates) Added a release notes entry if the change is major or breaking[ ] (Module updates) Added a release notes entry if the change is significant[ ] (Module addition) Added a release notes entry if adding a new NixOS module