New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better support for OpenBSD #4584

Closed
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@qbit
Copy link
Contributor

qbit commented Dec 10, 2017

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
Copy link
Member

faho left a comment

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.

@@ -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'

This comment has been minimized.

@faho

faho Dec 10, 2017

Member

Typo: "modifier".

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

This comment has been minimized.

@qbit

qbit Dec 10, 2017

Contributor

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

This comment has been minimized.

@faho

faho Dec 10, 2017

Member

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"'

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'

This comment has been minimized.

@faho

faho Dec 10, 2017

Member

More descriptions.

@@ -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 )'

This comment has been minimized.

@faho

faho Dec 10, 2017

Member

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

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

This comment has been minimized.

@faho

faho Dec 10, 2017

Member

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

This comment has been minimized.

@faho

faho Dec 10, 2017

Member

Use __fish_seen_subcommand_from.

@@ -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}\' )'

This comment has been minimized.

@faho

faho Dec 10, 2017

Member

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?

This comment has been minimized.

@qbit

qbit Dec 10, 2017

Contributor
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

This comment has been minimized.

@faho

faho Dec 10, 2017

Member

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 comment has been minimized.

@qbit

qbit Dec 10, 2017

Contributor

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

@@ -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 comment has been minimized.

@faho

faho Dec 10, 2017

Member

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

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

This comment has been minimized.

@faho

faho Dec 10, 2017

Member

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?

This comment has been minimized.

@qbit

qbit Dec 10, 2017

Contributor

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.)

This comment has been minimized.

@faho

faho Dec 10, 2017

Member

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?

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

This comment has been minimized.

@faho

faho Dec 10, 2017

Member

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

This comment has been minimized.

@qbit

qbit Dec 10, 2017

Contributor

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.

This comment has been minimized.

@faho

faho Dec 10, 2017

Member

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)

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

This comment has been minimized.

@faho

faho Dec 10, 2017

Member

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

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'):

This comment has been minimized.

@faho

faho Dec 10, 2017

Member

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.

This comment has been minimized.

@qbit

qbit Dec 10, 2017

Contributor

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..

This comment has been minimized.

@faho

faho Dec 10, 2017

Member

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)

qbit added some commits Dec 10, 2017

Add generic parser for /etc/man.conf
- Also rename get_paths_from_manpath() to get_paths_from_man_locations()

@qbit qbit force-pushed the qbit:master branch from 18f3aaa to 2562484 Dec 11, 2017

@qbit

This comment has been minimized.

Copy link
Contributor

qbit commented Dec 11, 2017

  • 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
Copy link
Member

faho left a comment

Nice work, a few remaining nits.

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." \

This comment has been minimized.

@faho

faho Dec 11, 2017

Member

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 ".".

'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."

This comment has been minimized.

@faho

faho Dec 11, 2017

Member

Show contents of state table or Show state table contents?

@@ -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 comment has been minimized.

@faho

faho Dec 11, 2017

Member

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)'
@@ -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}' '')'

This comment has been minimized.

@faho

faho Dec 11, 2017

Member

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").

# 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

This comment has been minimized.

@faho

faho Dec 11, 2017

Member

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.

@@ -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 has been minimized.

@faho

faho Dec 11, 2017

Member

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

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

@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 11, 2017

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 added some commits Dec 11, 2017

@qbit

This comment has been minimized.

Copy link
Contributor

qbit commented Dec 11, 2017

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

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

@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 12, 2017

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?

@faho

faho approved these changes Dec 12, 2017

@qbit

This comment has been minimized.

Copy link
Contributor

qbit commented Dec 12, 2017

Na, you can go for it :D

@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 12, 2017

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

@faho faho closed this Dec 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment