net/vnstat: Update to 1.17

[diizzy]

Summary

Update vnstat to 1.17
Drop individual directories for pid and log file
Don't pass path to pid file twice (cmd argument and config file)

[dor]

vnstat/Makefile

56

Creation of this directory is handled in the startprecmd. I'm guessing it was handled there as the user might change it and then you'd end up with a random empty directory hanging around.

vnstat/files/vnstat.in

64–65

Looks like this might chown the /var/run directory. I don't think this is what you actually wanted.
I'm guessing you'd want something like the following to create a writable file for the pidfile:

if [ ! -e ${vnstat_pidfile} ]; then
    install -o ${vnstat_user} -g ${vnstat_group} /dev/null ${vnstat_pidfile};
fi

[diizzy]

vnstat/files/vnstat.in

64–65

Yes, in its current state it's incorrect and not what I intended. Having a look at it again, since we're using directory root do we even need to use install at all for the pid file? Can't we just remove at altogether?

[dor]

vnstat/files/vnstat.in

64–65

/var/run won't be writable by $vnstat_user, so you have to do something to put a writable file in place. Using install seems to be a common pattern for doing that.

[diizzy]

vnstat/files/vnstat.in

64–65

Indeed but vnstat doesn't set up that before switching to its own user?

[dor]

vnstat/files/vnstat.in

64–65

The $vnstat_user is used by rc.subr(8) (check run_rc_command function and man page) to start the daemon as the specified user, so I don't believe that vnstatd will be starting as root and dropping to $vnstat_user, it will just be run as $vnstat_user immediately (via su(1)), and be unable to write out its PID unless a suitable file is already present.

[diizzy]

vnstat/files/vnstat.in

64–65

Well, that doesn't really work to answer my own question.
What you suggested seems to be the best way of handling it.

[diizzy]

Drop individual directories for pid and log file, thanks David O'Rourke for suggestions and reviewing.

[diizzy]

Remove directory dir in Makefile as it's handled by the in the startprecmd

[mat]

vnstat/Makefile

43–44

do-install-GUI-on:

55–56

remove echo.

56–58

Remove @ from here.

[diizzy]

vnstat/Makefile

43–44

To be clear replace the if statement with do-install-GUI-on: ?

55–56

Remove the line completely or what do you mean?
The handbook lists ECHO_MSG examples so I'm not sure what you're trying to point out.
https://www.freebsd.org/doc/en/books/porters-handbook/makefile-masterdir.html (for instance)

[mat]

As a side note, you may want to pass the rc file through devel/rclint to make sure it conforms to our coding style. (and through devel/hs-ShellCheck to make sure there are no lingering bugs.)

vnstat/Makefile

43–44

yes.

55–56

I mean remove the ECHO_MSG, almost nobody builds ports manually nowadays, I've been meaning to go trough the tree and remove those.
I'll look at the handbook and clean things up.

[diizzy]

Fix issues pointed out by mat@

rclint output: ERROR:root:[0]: No description included
shellcheck doesn't output any errors except "Double quote to prevent globbing and word splitting." which seems harmless

[mat]

vnstat/Makefile

36

Why +=?

38–41

Remove.

[diizzy]

More fixes that mat@ pointed out

[mat]

vnstat/Makefile

39–41

There is no need for all those \/ here, using / would be enough. Also, it would be better to do it all in one command, no need to run sed three times on the same file.

[diizzy]

vnstat/Makefile

39–41

I'm not very skilled at regexps so if you have a better way please provide one.

[mat]

vnstat/Makefile

39–41

Like I said, replace all the \/ with /. Then make it only one sed command, not three something like this:

${REINPLACE_CMD} -e "foo" \
                 -e "bar" \
                 the/file

[diizzy]

Superseded by commit r485333