Shell completion

[behrmann]

That's a rough draft I had lying around for a while now and polished up on the train. It needs more completers for actions and I haven't thought about whether the assumptions for the bash completion are sound and the fish completion is written looking just at the docs.

The idea is to be able to generate a completion script that distros can "build" during packaging and ship pre-built.

[bluca]

bash completion would be really nice, yes please

[keszybz]

This is pretty nice. If only we could make zsh work too :)

Some issues I notice in quick testing:

• we only allow one verb, so if there's a verby-looking word already present, we shouldn't propose verbs.
• most verbs don't accept options after the verb. Thus, mkosi build - should not complete our options. Either nothing or a general command completion like for sudo.

[behrmann]

This is pretty nice. If only we could make zsh work too :)

I have a few long train rides upcoming this week and I've been reading the rather dense zsh completion documentation ;)

[behrmann]

Pushed an updated version, making this just an command line option instead of a verb, this also sidesteps the shell autodetection. This version adds zsh, but I've still not tested zsh nor fish, since I forgot to install them before boarding my train.

I've kept the .write calls instead of print, since I find it nicer for the crufty strings this is building.

I've not yet thought about not proposing verbs multiple times, because in the case of the bash completion this will necessitate changing the approach for the completion completely, since the whole argument string needs to be scanned and I think this can't use the "stateless" approach of looking at the current and preceding option. No idea how fish and zsh handle this, but generally all three need a few more completion generations.

I think the shell check things are mostly gunk, except maybe the thing about mapfile, this needs looking into.

Please, nobody ask for another shell, bash and zsh are scary enough, I do hope nobody is still using csh or ksh... :)

[keszybz]

I hammered the zsh part until is started to behave.

[behrmann]

Another train ride another battle with shell completion. I tested the generated completions for bash, fish and zsh and at least they don't throw errors on me and complete the options I tried. The zsh version now has working choices, too, which didn't work for me during testing and the zsh version has the constant stuff factored out into a resource, like the bash version. Adding more completers might necessitate adding a resource for fish as well.

There's still a lot of room for improvement (boolean options that can also take various things that we parse as booleans, options that take comma- or otherwise separated lists), but the basic stuff is there.

The three completions are as similar as possible and as idiomatic as I was able to make them without being a user of fish or zsh (thanks @keszybz!). The bash version, while idiomatic and using what bash provides (it's O(1)!), will happily complete multiple verbs. That will need fixing, but that's a rewrite of the resource to parse the whole command line on every completion. I think I'd like to leave that for later.

Please give it a try to see what breaks :)

[DaanDeMeyer]

Can't we pass the shell for which to generate completions for as an argument and then make this a new verb completion? I don't like making this an option tbh.

[DaanDeMeyer]

if we made this a verb, we could also move all this logic into its own file completion.py

[keszybz]

Ooh, nice, under zsh, completions for -d and -t work perfectly.
The same under bash.
So I think this is functionally complete.

I don't really have an opinion on verb vs. option choice. Previously, I asked for the verb to be removed because it was had both an option and the verb. But having the verb without an option is also fine.

In my local testing, I get this strange result:

$ bin/mkosi --shell-completion=bash | less
...
declare -A _mkosi_compgen=([-C]=_mkosi_compgen_dirs [--directory]=_mkosi_compgen_dirs)

:[13]  + 2689831 suspended (tty input)  bin/mkosi --shell-completion=bash | 
        2689832 suspended (tty input)  less

It's stochastic, but happens ~80% of the time. I don't understand what is going on here, but I think it may be a bug in less. The output from mkosi seems to be just pure ASCII. Maybe zsh tries to do a read at exactly the wrong time and this causes … something?

[behrmann]

I have no strong feeling regarding whether this is an option or a verb, I'll change it back to a verb.

[behrmann]

Pushed a rework into a verb that also moves most of the stuff into a separate module. The CompGen enum, since it lives in Args, needs to stay in config, though, so as to not have a cyclic import.

[DaanDeMeyer]

Can we fix the shellchecks as well? I didn't review the generation stuff too closely but it all looks reasonable, the comments are mostly about decoupling this a little more so it's self contained.

Still need to test the fish completion

[keszybz]

I force-pushed with the changes that were requested. I want to get this merged… I thought it'd be a few small cleanups, but then it turns out to be more work. I hope that's OK.

[behrmann]

I thought it'd be a few small cleanups, but then it turns out to be more work. I hope that's OK.

Thanks! That's why I didn't get around to it this week, a few deadlines to keep.

Also, adjust the formatting in the bash resource to follow the usual style
with 'if something; then' on one line.

I gave you the style in zsh, give me bash. I strongly detest semicolons in shell scripts and the two line style is the way ALGOL intended it. ;)

Joke aside, the style with semicolon is more common, but I do think the two-line style is more readable, makes visually nicer blocks and looks nicer when the predicate or what one wants to loop over takes more than one line, which happens easily.

I'm not married to it, though, and the further rework to make the different CompGen usable can happen later, that is why the annotation for them was in config. Daan and I spoke about trying to look up the types on the Config object, but we don't need to wait for this, the current form should be good enough as a start and stylistic quibbles can be changed later.

[keszybz]

I undid the style change to ifs.

[keszybz]

OK, the tests pass and the comments have been addressed. Can we merge this and do follow-ups separately?

[DaanDeMeyer]

shellcheck is still failing. I'm fine with disabling the checks but that should be fixed before merging.

[DaanDeMeyer]

LGTM but CI should be green

[keszybz]

It is green, except for bogus comments from shellcheck that cannot be silenced.

Oh, OK, I guess this requires a longer explanation:
shellcheck could be silenced by inserting suppression comments into the scripts.
But we very do not want to do this, because that would mean that we cannot check the whole script. I added a test to check the whole script, and that passes without any warnings.

Sadly, differential shellcheck is not flexible enough here.

[keszybz]

koalaman/shellcheck#2411

[DaanDeMeyer]

@keszybz The differential-shellcheck github action has an exclude-path option. We can use that.

[behrmann]

I added a commit with a forward definition of the (hash) arrays, that should hopefully appease the differential shell check

[keszybz]

The differential-shellcheck github action has an exclude-path option. We can use that.

I looked at docs, but I didn't see it. That would work.

I added a commit with a forward definition of the (hash) arrays, that should hopefully appease the differential shell check

That also works…

[behrmann]

Dropped a shellcheck directive I accidentally committed.