I apologise for such delayed response. We're having major electricity
outages here and I couldn't find the possibility to address your review.
Nonetheless, here we are :)
On Wed, 2024-07-17 at 08:41 +0000, Michael Orlitzky wrote:
> On Wed, 2024-07-17 at 15:05 +0300, Zurab Kvachadze wrote:
> >
> > +NGINX_CONFIGFILE=${NGINX_CONFIGFILE:-/etc/nginx/nginx.conf}
> > +pidfile=${NGINX_PIDFILE:-/run/nginx.pid}
>
> [...]
>
> In fact, I would delete the NGINX_PIDFILE variable entirely, leaving it
> at /run/${RC_SVCNAME}.pid. There's no reason for anyone to change it.
> You can force the daemon to use that path with -g "pid ${pidfile}",
> relieving you of the responsibility to coordinate with the config file.
I haven't even known of this OpenRC feature! As you explained it, the
removal of NGINX_{CONFIG,PID}FILE totally makes sense. Should I also add
'-g "pid ${pidfile};"' to command_args for your example with
'nginx-internal' to work automatically without any conf.d tinkering?
Anyway, I will apply both (or only the first one) changes to the v2 of
patch series.
> > +depend() {
> > + need net
> > + use dns logger netmount
> > +}
>
> I don't think "need net" is right here because nginx starts fine on
> localhost or 0.0.0.0 until you configure it to use a specific address.
Initially, I've also wanted to remove the aforementioned line, but my
end decision was to leave it alone, because I wasn't really sure of its
uselessness. Now that you've explained it (and after reading the service
writing guide) I see that this directive is of no use and, thus, will be
removed in the second version of this MR.
> > +start_pre() {
> > + checkpath -d -o root:root /var/tmp/nginx || return 1
> > +}
> > +
>
> The old script had,
>
> if [ "${RC_CMD}" != "restart" ]; then
> configtest || return 1
> fi
>
> here. If "nginx -t" produces better error messages that plain nginx,
> that might be the reason. But otherwise it's redundant.
With nginx.eclass, the build flags now configure NGINX to store its
temporary files in /var/tmp/nginx. Yet, NGINX doesn't create it by
itself, but rather aborts its execution if /var/tmp/nginx doesn't exist.
This is why I added the 'checkpath' line. As for the configtest, it's
redundant.
When NGINX is executed, it checks the configuration, making it
unnecessary to check the same thing beforehand (the error reporting is
identical anyway). configtest just duplicates NGINX's work and this may
lead to twice the time to bring NGINX up, which may matter if large "geo
module bases" are used[1].
[1]: https://bugs.gentoo.org/481456#c0
