Consolidate all tools behind one subtool #160

whot · 2020-07-08T05:35:12Z

Fixes #151

This PR adds a new tool xkbcommon which is the git-style parent tool to start the various other tools. Already included are:

xkbcli
xkbcli list
xkbcli interactive-evdev
xkbcli interactive-wayland
xkbcli interactive-x11
xkbcli compile-keymap
xkbcli how-to-type

The infrastructure is copied from libinput which does the same thing with its tools. Having the forking approach makes code management easier, each subtool is still its own program and there's no requirement on programming language.

Man pages are generated via ronn which is a ruby program. scdoc looked interesting but can only handle stdin as input and I don't know how well forking a shell during meson's custom_target() works on macos and windows. Ruby at works on all three platforms.

This is still a draft, missing are:

how-to-type
more testing, it's been a day of frantic typing, something like libinput's tool option parsing test will be useful eventually
ideally our option parsing (esp. where we take RMLVO) should be identical across all tools but getopt_long() is conditional, so, urgh. See Why is getopt_long() optional? #161

Anyway, I'd appreciate a quick peek and some heads-up about anything that'd be a blocker here. Thanks.

whot · 2020-07-09T04:28:21Z

Updated, builds on Win/Macos now as well. Renamed to xkbcli and took the indirection of an interactive-debug subtool out. easier to just have this as top-level command.

whot · 2020-07-13T05:50:01Z

Rebased on top of #164

whot · 2020-07-13T05:53:54Z

uhm, I don't know what I have to do to start the CI, it's not running for some reason. I've rebased this on top of #164 and did a bit more cleanup, I'd rather have any nontrivial stuff as follow-up patches, this is getting a bit painful to rebase.

Notably: I've ditched the separate getopt vs getopt_long handling in meson.build with the assumption that if we have getopt.h we also have getopt_long - and where not we just don't build the tools (i.e. on Windows). If this becomes a necessity we can emulated getopt_long ourselves which is more sensible than ifdefing and conditionally building some tools.

Other than that I think this is ready, or at least I've stared at it long enough now to not see the bugs.

whot · 2020-07-14T02:04:14Z

Fixes tag added for #161 and the force-push caused the test suite to run, so that's a win.

bluetech

Awesome work.

I left some comments on the code.

Some larger comments:

Not sure about using ronn for the man pages (left a code comment).
I'm a bit concerned about installing these tools when they are statically linked instead of linking to the system xkbcommon. I think it's not appropriate to install like this... The issue is that some tools use private APIs. I propose we remove the install: true temporarily in this PR (in a revertible commit) and merge it, and handle this issue separately as it would require some changes probably.

.github/workflows/main.yml

tools/tools-common.c

tools/xkbcli-interactive-evdev.1.ronn

tools/how-to-type.c

tools/test_tool_option_parsing.py

whot · 2020-07-22T02:09:33Z

Windows test case failure is the same as in #165 (comment)

meson complains because this requires 0.50.0 and we don't require that. But since it defaults to false anyway, let's just omit it. Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

We cannot add to configh_data after this command so let's generate this last. Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

Windows doesn't have getopt.h. This would prevent building the tools but they are behind other checks that cause them to be disabled. The only tools that don't need getopt.h are interactive-wayland and interactive-x11 but neither is particularly useful on Windows. Just hide all tools behind the getopt check in preparation for the upcoming tool consolidation work. Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

This is the base tool, no subtools are currently connected so you only get help and version for now. The goal here is to have a git-like infrastructure where /usr/bin/xkbcli is the main tool, anything else will hide in libexec. The infrastructure for this is copied from libinput. Tools themselves will will be installed in $prefix/libexec/xkbcommon and the xkbcli tool forks off whatever argv[1] is after modifying the PATH to include the libexec dir. libinput has additional code for checking whether we're running this from the builddir but it's a bit iffy and it's usefulness is limited - if you're in the builddir anyway you can just run ./builddir/xkbcli-<toolname> directly. So for this code here, running ./builddir/xkbcli <toolname> will execute the one in the prefix/libexecdir. Since we want that tool available everywhere even where some of the subtools aren't present, we need to ifdef the getopt handling. man page generation is handled via ronn which is a ruby program but allows markdown for the sources. It's hidden behind a meson option to disable where downloading ronn isn't an option. The setup is generic enough that we can add other man-pages by just appending to the array. Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

The xkbcli tool usage help is ifdef'd out where the tool isn't built but the man page always includes all tools. Easier that way. Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

Requiring long options for this tool means it's immediately obvious what an invocation does, compare e.g. xkbcli interactive-evdev -gcd to the equivalent: xkbcli interactive-evdev --consumed-mode=gtk --enalbe-compose --report-state-changes This drops the evdev offset argument - that offset should never be anything other than 8, having this as argument here is more likely to confuse or produce misleading debugging logs. Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

This provides consistency with the other tools that now all take long options. Plus, it's more obvious to have the arguments spelled out. Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

On all platforms we build on where getopt.h is available, getopt_long is also available. Only Windows doesn't have either but that's no reason for us to differentiate between the two. If we need to special-case getopt vs getopt_long, it's probably better to implement our own cross-platform version of it and use that. Fixes xkbcommon#161 Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

A pytest wrapper around our xkbcli tool - copied from libinput. This calls our various xkbcli tools with varying options and check that they either succeed or return the right error code. The coverage is limited, it does not (and cannot) test for all possible combinations but it should provide a good red flag if we have inconsistent behavior or accidentally break some combination of flags. Meanwhile, we can at least assume that all our commandline arguments are parsed without segfaulting or worse. Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

tools/tools-common.c

bluetech · 2020-07-23T06:48:10Z

@whot Please let me know when it's ready again. (And also mark comments as "Resolved" so it's easier to see if anything remains).

Drop the ronn source files, check in the generated files instead. This gets rid of the ruby+gem+ronn toolchain requirement at the cost of having to edit raw man pages. ronn files are as-generated but with the preamble and generation date removed. The latter isn't important enough to keep, it'll just go stale for manually maintained files and it's not worth setting up a configure_file() just for that date. Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

whot · 2020-07-23T11:21:15Z

I propose we remove the install: true temporarily in this PR (in a revertible commit) and merge it, and handle this issue separately as it would require some changes probably.

Sorry, only just saw this: I'm not 100% sure on static linking rules in distros but given the binaries come from the same repo as the shared library, I'm not sure it classifies. Rather hard to update libxkbcommon.so without updating xkbcli in the process. It's cross-project linking that is usually a no-no. But if that's the killer for now, I can fix this as a follow-up in this tree tomorrow. Removing install and adding it back is almost more work (and since xkbcli won't do much when run in the builddir it's pointless anyway).

This is merely to fill in some NULL pointers anyway, we can just use the #defines we have available at build time. Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

The tools previously linked against a static version (by simply recompiling everythiong). This isn't necessary, we can link them against libxkbcommon.so. Only exception: The xbkcli-compile-keymap tool needs a private API for the --kccgst flag. Avoid this by disabling this flag in the installed tool and building the same tool, statically linked but not-installed. Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

whot · 2020-07-24T03:41:38Z

Ok, done. I removed the static linkage but this means we lose the --kccgst flag from xkbcli-compile-keymap. Given the intrinsics of how to look up the KcCGST component names, I don't think it's a good idea to re-implement this in the tools just for the sake of avoiding the private linkage.

tbh, I don't think we gain much here and the --kccgst flag is actually useful to debug issues in rules files IMO this is a net loss overall. One other way around it is to link the other tools against libxkbcommon.so and only statically link this one tool. Just so we save a few bytes of memory ;)

bluetech · 2020-07-25T12:10:38Z

This is merged now. Thanks for all the work @whot.

I added a commit on top f439ce1, please take a look and let me know if something seems wrong.

bluetech · 2020-07-25T13:40:23Z

(and some more commits after that)

whot force-pushed the wip/tool-consolidation branch 3 times, most recently from a5cde78 to e4e99c7 Compare July 9, 2020 04:01

This was referenced Jul 9, 2020

Combine demos to one binary #151

Closed

Allow for merged KcCGST files and a system path #162

Merged

whot force-pushed the wip/tool-consolidation branch from e4e99c7 to 6e36c6e Compare July 13, 2020 05:49

whot marked this pull request as ready for review July 13, 2020 05:50

whot requested a review from bluetech July 13, 2020 05:50

whot force-pushed the wip/tool-consolidation branch from 6e36c6e to 7dec5f1 Compare July 14, 2020 01:30

bluetech reviewed Jul 21, 2020

View reviewed changes

whot force-pushed the wip/tool-consolidation branch from 7dec5f1 to 1091ac3 Compare July 22, 2020 01:56

whot added 6 commits July 22, 2020 20:57

meson.build: drop install:false from configure_file

82645bb

meson complains because this requires 0.50.0 and we don't require that. But since it defaults to false anyway, let's just omit it. Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

meson.build: registry list is dependent on getopt_long

0f55bda

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

meson.build: move the config.h generation to the bottom

df6b826

We cannot add to configh_data after this command so let's generate this last. Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

meson.build: move registry-list tool down to the tools section

1712314

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

whot force-pushed the wip/tool-consolidation branch from 1091ac3 to 132fc1c Compare July 22, 2020 10:57

whot added 7 commits July 23, 2020 09:32

tools: install our tools as xkbcli subcommands

3097d32

The xkbcli tool usage help is ifdef'd out where the tool isn't built but the man page always includes all tools. Easier that way. Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

tools: consistently return 2 on invalid usage

6bb169a

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

tools: switch how-to-type to getopt_long

623eaba

This provides consistency with the other tools that now all take long options. Plus, it's more obvious to have the arguments spelled out. Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

tools: don't assert on 64+ commandline arguments, just return

e4343c2

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

tools: don't mangle the path for tools, just exec directly

7176b03

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

whot force-pushed the wip/tool-consolidation branch from 132fc1c to 32bf408 Compare July 22, 2020 23:52

bluetech reviewed Jul 23, 2020

View reviewed changes

tools/tools-common.c Outdated Show resolved Hide resolved

whot force-pushed the wip/tool-consolidation branch from 20feda2 to 41ec5b1 Compare July 23, 2020 11:20

whot added 2 commits July 24, 2020 13:38

tools: avoid use of a private api

bd05bc1

This is merely to fill in some NULL pointers anyway, we can just use the #defines we have available at build time. Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

bluetech mentioned this pull request Jul 25, 2020

Add version of linked library in xkbcli #171

Closed

bluetech closed this Jul 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate all tools behind one subtool #160

Consolidate all tools behind one subtool #160

whot commented Jul 8, 2020 •

edited

whot commented Jul 9, 2020

whot commented Jul 13, 2020

whot commented Jul 13, 2020

whot commented Jul 14, 2020

bluetech left a comment

whot commented Jul 22, 2020

bluetech commented Jul 23, 2020

whot commented Jul 23, 2020

whot commented Jul 24, 2020

bluetech commented Jul 25, 2020

bluetech commented Jul 25, 2020

Consolidate all tools behind one subtool #160

Consolidate all tools behind one subtool #160

Conversation

whot commented Jul 8, 2020 • edited

whot commented Jul 9, 2020

whot commented Jul 13, 2020

whot commented Jul 13, 2020

whot commented Jul 14, 2020

bluetech left a comment

Choose a reason for hiding this comment

whot commented Jul 22, 2020

bluetech commented Jul 23, 2020

whot commented Jul 23, 2020

whot commented Jul 24, 2020

bluetech commented Jul 25, 2020

bluetech commented Jul 25, 2020

whot commented Jul 8, 2020 •

edited