Add console script entry point #1745
Conversation
|
Are the shell scripts needed (except bash_completion), do they need to stay in FB? Rewrite? Move somewhere else? What about the two test_* files? |
so we loose the Some random questions comming into mind:
|
Well, they were needed at some point.
Rather yes, it's good to have a place for things like this and when it is closely related to FB they are good here. We don't want to have people redo them all the time they need something similar. AFAIK this are tools @m4rc1e and @felipesanches are working with occasionally.
Hard to justify, isn't it? Since they just work already. Sounds like an exercise. I'd prefer an idea how we can transition from old to new, without having to care too much about the old. This is in
Eventually I'd prefer a single With shell scripts (and others), I see the problem that the python module system is getting to its limits. Adding Non-Code Files could help (it's not python so it's Non-Code heh?) |
Huh? We don't if we leave
Nothing will happen to them I guess? The directory listing of my fontbakery venv on Windows looks like this: All the current bin/* stuff is there, so you could ignore my work here even if it's merged and continue doing what you were doing before. If there should be only one script that relays everything, all shell scripts have to be rewritten or discarded.
Your hint is correct ;)
As hard as making a new module in Lib/fontbakery/commands (and adding it in cli.py). The entry point stuff gets rid of the boilerplate seen in
I didn't need to touch the specification checking scripts, not sure about the others... They'd have to be "ported" to commands, though. |
How so? What about |
|
Uhm, did I understand correctly that you would want fontbakery to export just one script that does everything? I guess you can install shell scripts somewhere in Lib and Popen them if you want to... would need a command in commands/ though. Might as well leave the script in bin/ then. |
Sure, why not? We already have it (last time I checked)
That's why I was referring to Adding Non-Code Files
I'd build it into
Yeah, there's that hack in https://github.com/googlefonts/fontbakery/blob/master/setup.py#L41 which installs all scripts in the current path. It's OK if you are in a virtualenv, but it's kind of ugly if you are not. So having all behind a single command is like a service to our users to keep their computers clean. |
|
Alrighty then, will write something and push it here. |
|
Quick check: does build-contributors work for you? I get a Unicode en/decode error upon writing doc. Looks like this is a one way of preparing Python 3 support ;) |
non-code as in data files. A shell script is code, should go with |
|
Yeah, so I think the tricky part is to get pip into installing the scripts (Non-Code Files) and to figure out where to read them from. Name collisions may be a thing at some point in the future, i.e. to add power shell based scripts |
|
I think that would be more elegantly handled by installing these scripts in the path. Powershell and cmd.exe have a list of suffixes specified that describe executables and that the user can leave out. Executing "venv/scripts/activate" in Powershell automatically chooses the file with the ".ps1" suffix |
It's not code for pip I think. And I'm not sure if shell scripts are scripts for pip. |
Lib/fontbakery/cli.py
Outdated
| sys.argv[0] += " " + sys.argv[1] | ||
| del sys.argv[1] # Make this indirection less visible for subcommands. | ||
| cmd = importlib.import_module(subcommand) | ||
| sys.exit(cmd.main()) |
There was a problem hiding this comment.
instead of importlib.import_module and then sys.exit(cmd.main()), you could use runpy.run_module as in:
https://github.com/fonttools/fonttools/blob/88f495dd68a20c1e16043e57e072324c628a8dc9/Lib/fontTools/__main__.py#L28-L29
https://docs.python.org/3/library/runpy.html#runpy.run_module
There was a problem hiding this comment.
that's basically like doing python -m some_module
|
"scripts" can be anything. setuptools simply chmod +x and copy them to the bin or Scripts folder |
|
I can "port" the Python scripts over, the shell scripts could stay in "scripts" until they're deprecated or replaced. The path will be less cluttered either way. |
Lib/fontbakery/cli.py
Outdated
|
|
||
|
|
||
| def main(args=None): | ||
| subcommands = { |
There was a problem hiding this comment.
can we (maybe later) add an auto discovery mechanism to this. It would spare us from having to register each command. Also we have that kind of thing in bin/fontbakery so this would be a regression.
There was a problem hiding this comment.
Sure. I just need to look into discovering the submodules of a module :)
In essence, is having |
ehm, well almost anything, but they must be text, not binary, see: (even though the afdko hacks its way to it somehow and installs binary native executables as scripts, but I wouldn't recommend it) |
(stupid keyboard, I keep hitting "Comment" before finishing to write my comments..) it's certainly cool, but doesn't work on a system where bash or powershell or whatever shell is not present. Maybe you could check if the shell is in the PATH and if not print a nice error message. |
Ehm, it is? Just a thought on the way forward because you seem to be wary of additional work with little ROI. |
|
What's this fb_summary_report thing for? Should I carry it over, too? Looks like a one-off script. |
|
Just trying to figure out what features to leave behind and for what reason. Me being wary about ROI was discussing doing
Looks old. Does it still work? |
So we can either dump the feature or just issue the warning you suggested and use that occasion to replace it with a better compatible python version. After all, that would be a case where we know it's justified (because someone wants to use it). I like the shell scripts, because I believe it's sometimes just very fast to make them. And if there's a low entry barrier to extending Font Bakery, it sounds good to me at first. We can also over engineer a bit and if there are two commands |
No idea. It expects a certain directory with certain files in it but has no documentation. |
|
I wrote that summary report in order to get a grasp of the overall quality of the gfonts collection by running check-googlefonts locally on the full collection with the --json output mode saving the individual reports in that directory. Then I used this tiny python script to compute the totals, displaying which files had problems and how many errors were reported. I think this might still be useful, but it is also pretty quick to draft from scratch if anyone really needs that kind of summary report. This is the kind of thing that will be much better done with the fontbakery-dashboard, so I would actually be happy to see this deleted now. |
|
@felipesanches we could put it in a Also, for Google Fonts, the dashboard will do it. But not everyone will set up the dashbooard for their own collection, at first at least unless it becomes easy and cheap. |
|
yep! My explanation from above fills the "we lack documentation" gap ;-) |
🤔 Documentation is sometimes like a good wine, it needs to ripen. LOL |
|
This is what @madig would expect with ( (BTW, should we remove the |
|
I would simply remove "There is a total of " but keep the rest of the info... |
|
Regarding the |
I had a quick look, e.g. in @check(id='com.google.fonts/check/013')
def com_google_fonts_check_013(ttFonts):
"""Fonts have equal unicode encodings?"""Will also run with an empty list of @check(
id='com.google.fonts/check/013',
conditions=['ttFonts'])
def com_google_fonts_check_013(ttFonts):
"""Fonts have equal unicode encodings?""" |
|
Cure LGTM
|
|
Hrm. |
In what case, without
That looks to me like it was always just a cheap hack in |
Basically, argparse should explode when it doesn't get what it wants. That will exit nicely.
check-specification and check-ufo-sources exit as expected, so, uh, would have to look into it. |
But argparse is getting what it wants. Read the source, Luke.
|
|
Oops, overlooked your message above. I think |
|
In any case, we should use
Or hack the argument parser in a way that it knows when to expect fonts. A new command must share some thngs with |
Instead of hand-rolling a main "fontbakery" script.
Additionally, fb_summary_report.py was deemed obsolete by Felipe.
Also reformat some files along the way.
All files in Lib/fontbakery/commands are now discovered as commands. `some_thing.py` will translate to the command `fontbakery some-thing`.
|
Mergy merge? 👷🔧 |
|
thanks, @madig and @graphicore ! |
| @@ -192,6 +192,9 @@ def main(specification=None, values=None): | |||
| values_[key] = getattr(args, key) | |||
|
|
|||
| try: | |||
| if not args.fonts: | |||
| raise ValueValidationError, "No files to check specified." | |||
There was a problem hiding this comment.
Hmm, I dislike this for two reasons:
- now a spec has to have a font's argument,
check-specificationwas clean of that before and specifications could define their dependencies much more freely. - There's a mechanism that raises exactly this error and ends the script (with this PR also with
sys.exit(1)So why do it two times?
Instead of hand-rolling a main "fontbakery" script (c.f. #1738). This is a request for comments -- should I continue down this path?
The various Python bin/fontbakery-* scripts could be added as more entry points if you really want to keep e.g. fontbakery-check-* instead of having just one fontbakery script that serves as an entry point or everything else.
(Ignore the check-ufo-sources thing for now, will remove before merging if my other branch isn't merged first.)