Better support for OpenBSD #4584

qbit · 2017-12-10T16:38:01Z

Description

This PR adds support for parsing man.conf, interfaces and packages on OpenBSD. It also brings in a set of completions for OpenBSD tools.

TODOs:

Changes to fish usage are reflected in user documentation/manpages.
Tests have been added for regressions fixed
User-visible changes noted in CHANGELOG.md

faho

I like what I'm seeing so far.

Aside from a few nitpicks, I'd like to have this separated into more commits. Ideally one per added completion or change to an existing thing.

faho · 2017-12-10T16:59:45Z

share/completions/pfctl.fish

@@ -0,0 +1,4 @@
+
+complete -c pfctl -s s --description 'Show a filter parameter by modifyer' -xa 'queue rules Anchors states Sources info labesl timeouts memory Tables ospf Interfaces all'


Typo: "modifier".

Also, what do these arguments mean? Descriptions here would be nice.

Do you mean more description in the --description section? or a comment for each option describing a bit more?

A comment for each option. You can either use a function like

function __fish_pfctl_filter_modifiers printf '%s\t%s\n' queue "something to describe queue" printf '%s\t%s\n' rules "something to describe rules" # .... end

or something like

complete -c pfctl -s s --description 'Show a filter parameter by modifyer' -xa \ 'queue\t"something to describe queue" rules\t"something for rules"'

faho · 2017-12-10T17:00:02Z

share/completions/pfctl.fish

+
+complete -c pfctl -s s --description 'Show a filter parameter by modifyer' -xa 'queue rules Anchors states Sources info labesl timeouts memory Tables ospf Interfaces all'
+complete -c pfctl -s F --description 'Flush the filter params specified by mod' -xa 'rules states Sources info Tables ospf all'
+complete -c pfctl -s T --description 'Table command' -xa 'kill flush add delete expire replace show test zero'


More descriptions.

faho · 2017-12-10T17:01:22Z

share/completions/rcctl.fish

@@ -0,0 +1,4 @@
+
+complete -c rcctl -xa 'check ls reload restart stop start disable enable' -n 'not __fish_seen_subcommand_from list check ls reload restart stop start enable disable'
+complete -c rcctl -n '__fish_seen_subcommand_from check reload restart stop start enable disable' -xa '( ls /etc/rc.d )'


Instead of ls, use printf. I.e. (set -l files /etc/rc.d/*; printf "%s\n" $files).

faho · 2017-12-10T17:02:24Z

share/completions/signify.fish

@@ -0,0 +1,4 @@
+complete -c signify -n '__fish_use_subcommand' -s C --description 'Verify a signed checksum list'


__fish_use_subcommand ignores things that start with a dash, so this would have no effect.

Use __fish_seen_subcommand_from.

faho · 2017-12-10T17:04:00Z

share/completions/vmctl.fish

@@ -0,0 +1,4 @@
+
+complete -c vmctl -xa 'console create load log reload reset start status stop pause unpause send receive' -n 'not __fish_seen_subcommand_from list console create load log reload reset start status stop pause unpause send receive'
+complete -c vmctl -n '__fish_seen_subcommand_from console reload reset start status stop pause unpause send receive' -xa '( vmctl status | grep -v NAME | awk \'{print $NF}\' )'


Preferably, this'd use string, since it's a builtin. The grep is simple - string match -v. I never really learned awk - what does the output look like and what are you trying to get out?

vmctl status ID PID VCPUS MAXMEM CURMEM TTY OWNER NAME 1 - 1 512M - - qbit ssb 2 - 1 1.0G - - qbit chrome 3 - 1 512M - - qbit alpine 4 - 1 512G - - qbit void

So you want the last field there? Yeah, that's something that isn't quite as nice with string.

Try vmctl status | string match -v 'NAME' | string replace -r '^(\s+\S+\s+){7}' ''

(Obviously the "NAME" part isn't great because IIUC you could have a VM called "NAME-SOMETHING" - what you'd really want is something like tail -n +2, but we can probably live with that)

This works if i do string match -e -v. Also I switched "NAME" to "MAXMEM" - because that's a super lame vm name ;D

faho · 2017-12-10T17:06:21Z

share/functions/__fish_print_packages.fish

@@ -35,6 +35,17 @@ function __fish_print_packages
        return
    end

+    # pkg_info on OpenBSD provides versioning info which we want for
+    # installed packages
+    if begin


This begin and end is unnecessary since about 2.3.0. Now and and similar bind to if.

faho · 2017-12-10T17:07:21Z

share/functions/__fish_print_packages.fish

+    # installed packages
+    if begin
+            type -q -f pkg_add
+            and test (uname) = "OpenBSD"


I like not using uname if we can somehow get away with it. Since we currently have no path for pkg_add, can we just remove this condition?

IMO there needs to be some way to distinguish between pkg_* variants - for example pkg_add exists in pkgsrc and FreeBSD (this might be replaced with pkg by now.. not entirely sure.)

Yeah, but we don't support those currently. We do support FreeBSD's pkg, so if they still have pkg_add and friends, we just need to make sure that pkg is checked first.

And if they have pkg_*, do they differ in ways that are meaningful here?

faho · 2017-12-10T17:07:54Z

share/functions/__fish_print_packages.fish

+            and test (uname) = "OpenBSD"
+        end
+	# pkg_info can get blocked if there is another instance of pkg_* running.
+        ls -1 /var/db/pkg


I'd again replace the ls with set -l files /var/db/pkg/* and printf '%s\n' $files.

Is there a "fish" way of removing the path from that output? %s\n ends up having /var/db/pkg/yquake2-7.10 - which will break pkg_* tools.

Ah, yes, I understand now.

Instead of printf, use string replace '/var/db/pkg/' '' -- $files.

(Note that all this breaks on files with newlines, but we don't support that with completions in general and in practice it simply does not matter)

faho · 2017-12-10T17:08:28Z

share/functions/__fish_print_packages.fish

+            type -q -f pkg_add
+            and test (uname) = "OpenBSD"
+        end
+	# pkg_info can get blocked if there is another instance of pkg_* running.


Is this why you're not calling pkg_info here? That should probably be stated outright.

faho · 2017-12-10T17:10:32Z

share/tools/create_manpage_completions.py

-        parent_paths = manpath.decode().strip().split(':')
+        if os.uname()[0] == 'OpenBSD':
+            parent_paths = ['/usr/share/man', '/usr/X11R6/man', '/usr/local/man']
+            if os.path.isfile('/etc/man.conf'):


Does anyone else use another incompatible /etc/man.conf?

If nobody does, we can just try this after we've tried to call the tools.

~~I am not entirely sure if anyone else uses it.~~ Looks like at least CentOS uses it.. I can't tell if it has differences in format from the man page they have..

I can't find anything on centos (which would be Red Hat), but it seems that FreeBSD and macOS use it as well.

See the FreeBSD manpage. Given that that uses a directive called "MANPATH" where OpenBSD uses "manpath", we could just try reading either (i.e. "the word manpath, case-insensitively") after we've exhausted the programs (which should still be preferred because they're more likely to get weird edgecases right). If that still doesn't work, we could still fallback to the current default.

As you might have guessed by now, I have a certain philosophy when it comes to cross-platform support, in that I really don't like checking for the specific platform name via uname or similar. I very much prefer feature-testing if at all possible. This allows us to e.g. do proper completion for GNU utilities even on systems where they aren't usually installed. (One interesting case where we can't do that is killall, because we absolutely cannot invoke it on Solaris - because it actually kills all)

- Also rename get_paths_from_manpath() to get_paths_from_man_locations()

qbit · 2017-12-11T14:55:46Z

Separated into more commits.
Typo / descriptions.
Less use of unix utilities in favour of fish built-ins.
Use __fish_seen_subcommand_from.
Generic ifconfig parsing for BSDs.
Parse man.conf if it exists.

I think I got everything.

faho

Nice work, a few remaining nits.

faho · 2017-12-11T15:56:43Z

share/completions/pfctl.fish

+complete -c pfctl -s s --description 'Show a filter parameter by modifier' -xa \
+	'queue\t"Show the currently loaded queue definitions." \
+	rules\t"Show the currently loaded filter rules." \
+	Anchors\t"Show the currently loaded anchors directly attached to the main ruleset." \


Descriptions should be as short as possible, so we can ideally fit more columns - Show anchors attached to main ruleset?

Also, I prefer no trailing ".".

faho · 2017-12-11T15:57:15Z

share/completions/pfctl.fish

+	'queue\t"Show the currently loaded queue definitions." \
+	rules\t"Show the currently loaded filter rules." \
+	Anchors\t"Show the currently loaded anchors directly attached to the main ruleset." \
+	states\t"Show the contents of the state table."


Show contents of state table or Show state table contents?

faho · 2017-12-11T15:59:50Z

share/completions/rcctl.fish

@@ -0,0 +1,3 @@
+
+complete -c rcctl -xa 'check ls reload restart stop start disable enable' -n 'not __fish_seen_subcommand_from list check ls reload restart stop start enable disable'
+complete -c rcctl -n '__fish_seen_subcommand_from check reload restart stop start enable disable' -xa '(set -l files /etc/rc.d/*; string replace '/etc/rc.d/' '' -- $files)'


This probably doesn't work properly because of the quoting - remember, you're already inside single quotes, so if you're going to use them you need to escape them (\'). Since that confuses some syntax highlighters, I usually use double-quotes in cases like these. I.e.:

-xa '(set -l files /etc/rc.d/*; string replace "/etc/rc.d/" "" -- $files)'

faho · 2017-12-11T16:00:49Z

share/completions/vmctl.fish

@@ -0,0 +1,4 @@
+
+complete -c vmctl -xa 'console create load log reload reset start status stop pause unpause send receive' -n 'not __fish_seen_subcommand_from list console create load log reload reset start status stop pause unpause send receive'
+complete -c vmctl -n '__fish_seen_subcommand_from console reload reset start status stop pause unpause send receive' -xa '(vmctl status | string match -e -v 'MAXMEM' | string replace -r '^(\s+\S+\s+){7}' '')'


Similarly with the quotes. I think those curly braces will actually be stripped (since they are outside of quotes, so fish evaluates them as a brace-expansion - echo {7} prints just "7").

faho · 2017-12-11T16:03:03Z

share/functions/__fish_print_packages.fish

+    # installed packages but, calling it directly can cause delays in
+    # returning information if another pkg_* tool have a lock.
+    # Listing /var/db/pkg is a clean alternative.
+    if begin


Again: This begin; end pair can be removed.

if type -q -f pkg_add # which is just `command -sq pkg_add` and test (uname) = "OpenBSD"

will do the right thing.

I still maintain though that the uname check can be removed as well.

faho · 2017-12-11T16:03:30Z

share/tools/create_manpage_completions.py

@@ -876,10 +875,18 @@ def get_paths_from_manpath():
        manpath, err_data = proc.communicate()
        parent_paths = manpath.decode().strip().split(':')
    if (not parent_paths) or (proc and proc.returncode > 0):
-        # HACK: Use some fallback in case we can't get anything else.
+        # HACK: Use some fallbacks in case we can't get anything else.
        # `mandoc` does not provide `manpath` or `man --path` and $MANPATH might not be set, so just use the default for mandoc (minus /usr/X11R6/man, because that's not relevant).
        # The alternative is reading its config file (/etc/man.conf)


This comment needs to be updated - we are reading the mandoc config file.

faho · 2017-12-11T16:06:06Z

Separated into more commits.

Just the way I wanted it, thanks! Would it be possible to add any remaining changes as one commit per thing as well? Either that or squash them on top of the relevant commits yourself.

I think I got everything.

Almost.

qbit · 2017-12-11T23:03:51Z

Updated - I was hesitant to remove the uname checks because there was more than just the one for OpenBSD - Now there are none.

fish-shell/fish-shell#4584

faho · 2017-12-12T18:10:42Z

Yes, this seems good to merge. Nice work!

I've just noticed that you've signed all your commits. I'd like to squash them (well, git rebase --interactive, but that doesn't roll off the tongue quite so nice) together so we have one commit per "thing", but if I do that I'm going to trample your signatures (or rather I'd create new commit objects that I obviously can't sign with your key).

So would you like to do that to keep the signatures or should I do it?

qbit · 2017-12-12T18:13:44Z

Na, you can go for it :D

faho · 2017-12-12T18:28:20Z

Merged as 277cd30..75e17e0. Thank you very much!

faho requested changes Dec 10, 2017

View reviewed changes

qbit added 11 commits December 10, 2017 10:35

add ffs to known filesystems

332be75

Add generic parser for /etc/man.conf

6f4ead2

- Also rename get_paths_from_manpath() to get_paths_from_man_locations()

parse *BSD interface names

ee14a7d

add package parsing for OpenBSD

b140a8a

add OpenBSD pfctl completion

d52572b

add pkg_add completions for OpenBSD

d3bd5b4

add pkg_delete completions for OpenBSD

d7ef7f0

add pkg_info completions for OpenBSD

cc7ee74

add rcctl completions for OpenBSD

52c2933

add vmctl completions for OpenBSD

42748cf

add signify completions for OpenBSD

2562484

qbit force-pushed the master branch from 18f3aaa to 2562484 Compare December 11, 2017 14:55

faho requested changes Dec 11, 2017

View reviewed changes

faho added enhancement release notes Something that is or should be mentioned in the release notes labels Dec 11, 2017

faho added this to the fish-3.0 milestone Dec 11, 2017

qbit added 5 commits December 11, 2017 16:01

shorten descriptions

8db1c75

fix quoting

5808fe9

fix quoting

54edaba

remove uname checks

f0962e7

update comment to reflect checked locations

81dbbc4

qbit added a commit to jasperla/openbsd-wip that referenced this pull request Dec 11, 2017

sync diffs with upstreamed:

952d258

fish-shell/fish-shell#4584

faho approved these changes Dec 12, 2017

View reviewed changes

faho closed this Dec 12, 2017

github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020

		@@ -0,0 +1,4 @@

		complete -c pfctl -s s --description 'Show a filter parameter by modifyer' -xa 'queue rules Anchors states Sources info labesl timeouts memory Tables ospf Interfaces all'

		@@ -0,0 +1,4 @@

		complete -c rcctl -xa 'check ls reload restart stop start disable enable' -n 'not __fish_seen_subcommand_from list check ls reload restart stop start enable disable'
		complete -c rcctl -n '__fish_seen_subcommand_from check reload restart stop start enable disable' -xa '( ls /etc/rc.d )'

		@@ -0,0 +1,4 @@
		complete -c signify -n '__fish_use_subcommand' -s C --description 'Verify a signed checksum list'

		@@ -0,0 +1,4 @@

		complete -c vmctl -xa 'console create load log reload reset start status stop pause unpause send receive' -n 'not __fish_seen_subcommand_from list console create load log reload reset start status stop pause unpause send receive'
		complete -c vmctl -n '__fish_seen_subcommand_from console reload reset start status stop pause unpause send receive' -xa '( vmctl status \| grep -v NAME \| awk \'{print $NF}\' )'

		@@ -0,0 +1,3 @@

		complete -c rcctl -xa 'check ls reload restart stop start disable enable' -n 'not __fish_seen_subcommand_from list check ls reload restart stop start enable disable'
		complete -c rcctl -n '__fish_seen_subcommand_from check reload restart stop start enable disable' -xa '(set -l files /etc/rc.d/*; string replace '/etc/rc.d/' '' -- $files)'

Better support for OpenBSD #4584

Better support for OpenBSD #4584

Conversation

qbit commented Dec 10, 2017

Description

TODOs:

faho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qbit Dec 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qbit commented Dec 11, 2017

faho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faho commented Dec 11, 2017

qbit commented Dec 11, 2017

faho commented Dec 12, 2017 • edited

qbit commented Dec 12, 2017

faho commented Dec 12, 2017

qbit Dec 10, 2017 •

edited

faho commented Dec 12, 2017 •

edited