package/nginx: fix NGINX pidfile handling systemd

Based on https://git.launchpad.net/ubuntu/+source/nginx/plain/debian/patches/nginx-fix-pidfile.patch
Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/nginx/+bug/1581864
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=876365

Upstream bug: (deferred fix)
https://trac.nginx.org/nginx/ticket/1897?cversion=0&cnum_hist=2

Signed-off-by: Matthew Weber <matthew.weber@collins.com>
---
 .../0010-Fix-NGINX-pidfile-handling.patch     | 105 ++++++++++++++++++
 1 file changed, 105 insertions(+)
 create mode 100644 package/nginx/0010-Fix-NGINX-pidfile-handling.patch

Matthew, All,

On 2021-05-21 09:41 -0500, Matthew Weber via buildroot spake thusly:
> Based on https://git.launchpad.net/ubuntu/+source/nginx/plain/debian/patches/nginx-fix-pidfile.patch
> Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/nginx/+bug/1581864
> Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=876365
> 
> Upstream bug: (deferred fix)
> https://trac.nginx.org/nginx/ticket/1897?cversion=0&cnum_hist=2

So, basically, upstream refused to fix that bug, because (re-ordered so
that what I believe most important gets listed first):

  - the believe the Ubuntu patch (which this is based on) is incorrect:
    "The patch suggested in the Ubuntu bugtracker to write pidfile in
    the parent process looks wrong. In particular, it does so with the
    wrong umask, which is set in the child process only."

  - they believe the synchronisation described in systemd's doc is too
    complex:
    "The patch to synchronize things as recommended bysystemd's
    daemon(7) manpage [...] wasn't committed as the change is considered
    too complex for the problem it solves"

  - they believe the message is innocuous (I suppose because systemd
    will loop to TERM then KILL all proceses in a cgroup, thus actually
    and eventually ending the nginx daemon).

  - a then-future version of systemd should include support for that
    situation with new the new unit directive "Type=pid-file"

 - a few various lesser interesting (to us) reasons

However, there seems to be an easy workaround: complement the systemd
unit with:

    ExecStartPost=/bin/sleep 0.1

(or even a bigger number if we want to support slow systems).

Given that the patch on the code was rejected by upstream, I am wary to
bundle it in Buildroot, because we will supposedly have to carry it
forever. In this respect, tweaking the ystemd unit seems a leser burden.

However, what about the systemd situation: is the reference
Type=pid-file feautre now implemented? Do we have it in the version
packaged in Buildroot?

If so, maybe we could then tweak the unit to make use of it, instead of
the sleep workaround?

Regards,
Yann E. MORIN.

> Signed-off-by: Matthew Weber <matthew.weber@collins.com>
> ---
>  .../0010-Fix-NGINX-pidfile-handling.patch     | 105 ++++++++++++++++++
>  1 file changed, 105 insertions(+)
>  create mode 100644 package/nginx/0010-Fix-NGINX-pidfile-handling.patch
> 
> diff --git a/package/nginx/0010-Fix-NGINX-pidfile-handling.patch b/package/nginx/0010-Fix-NGINX-pidfile-handling.patch
> new file mode 100644
> index 0000000000..9a50118434
> --- /dev/null
> +++ b/package/nginx/0010-Fix-NGINX-pidfile-handling.patch
> @@ -0,0 +1,105 @@
> +From 2491379370059c7b9a2b956a49c90a9de55f5dcb Mon Sep 17 00:00:00 2001
> +From: Tj <ubuntu@iam.tj>
> +Date: Fri, 21 May 2021 09:35:53 -0500
> +Subject: [PATCH] Fix NGINX pidfile handling
> +
> +Based on https://git.launchpad.net/ubuntu/+source/nginx/plain/debian/patches/nginx-fix-pidfile.patch
> +Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/nginx/+bug/1581864
> +Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=876365
> +Last-Update: 2020-06-24
> +
> +Upstream bug: (deferred fix)
> +https://trac.nginx.org/nginx/ticket/1897?cversion=0&cnum_hist=2
> +
> +Signed-off-by: Tj <ubuntu@iam.tj>
> +Signed-off-by: Matthew Weber <matthew.weber@collins.com>
> +---
> + src/core/nginx.c         | 24 +++++++++++++++++++++---
> + src/os/unix/ngx_daemon.c |  8 ++++++--
> + 2 files changed, 27 insertions(+), 5 deletions(-)
> +
> +diff --git a/src/core/nginx.c b/src/core/nginx.c
> +index 9fcb0eb..083eba1 100644
> +--- a/src/core/nginx.c
> ++++ b/src/core/nginx.c
> +@@ -338,14 +338,21 @@ main(int argc, char *const *argv)
> +         ngx_process = NGX_PROCESS_MASTER;
> +     }
> + 
> ++    /* tell-tale to detect if this is parent or child process */
> ++    ngx_int_t child_pid = NGX_BUSY;
> ++
> + #if !(NGX_WIN32)
> + 
> +     if (ngx_init_signals(cycle->log) != NGX_OK) {
> +         return 1;
> +     }
> + 
> ++    /* tell-tale that this code has been executed */
> ++    child_pid--;
> ++
> +     if (!ngx_inherited && ccf->daemon) {
> +-        if (ngx_daemon(cycle->log) != NGX_OK) {
> ++        child_pid = ngx_daemon(cycle->log);
> ++        if (child_pid == NGX_ERROR) {
> +             return 1;
> +         }
> + 
> +@@ -358,8 +365,19 @@ main(int argc, char *const *argv)
> + 
> + #endif
> + 
> +-    if (ngx_create_pidfile(&ccf->pid, cycle->log) != NGX_OK) {
> +-        return 1;
> ++    /* If ngx_daemon() returned the child's PID in the parent process
> ++     * after the fork() set ngx_pid to the child_pid, which gets
> ++     * written to the PID file, then exit.
> ++     * For NGX_WIN32 always write the PID file
> ++     * For others, only write it from the parent process */
> ++    if (child_pid < NGX_OK || child_pid > NGX_OK) {
> ++	ngx_pid = child_pid > NGX_OK ? child_pid : ngx_pid;
> ++        if (ngx_create_pidfile(&ccf->pid, cycle->log) != NGX_OK) {
> ++            return 1;
> ++	}
> ++    }
> ++    if (child_pid > NGX_OK) {
> ++        exit(0);
> +     }
> + 
> +     if (ngx_log_redirect_stderr(cycle) != NGX_OK) {
> +diff --git a/src/os/unix/ngx_daemon.c b/src/os/unix/ngx_daemon.c
> +index 385c49b..3719854 100644
> +--- a/src/os/unix/ngx_daemon.c
> ++++ b/src/os/unix/ngx_daemon.c
> +@@ -7,14 +7,17 @@
> + 
> + #include <ngx_config.h>
> + #include <ngx_core.h>
> ++#include <unistd.h>
> + 
> + 
> + ngx_int_t
> + ngx_daemon(ngx_log_t *log)
> + {
> +     int  fd;
> ++    /* retain the return value for passing back to caller */
> ++    pid_t pid_child = fork();
> + 
> +-    switch (fork()) {
> ++    switch (pid_child) {
> +     case -1:
> +         ngx_log_error(NGX_LOG_EMERG, log, ngx_errno, "fork() failed");
> +         return NGX_ERROR;
> +@@ -23,7 +26,8 @@ ngx_daemon(ngx_log_t *log)
> +         break;
> + 
> +     default:
> +-        exit(0);
> ++        /* let caller do the exit() */
> ++        return pid_child;
> +     }
> + 
> +     ngx_parent = ngx_pid;
> +-- 
> +2.17.1
> +
> -- 
> 2.17.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

On 26/05/2021 22:48, Yann E. MORIN wrote:
> Matthew, All,
> 
> On 2021-05-21 09:41 -0500, Matthew Weber via buildroot spake thusly:
>> Based on https://git.launchpad.net/ubuntu/+source/nginx/plain/debian/patches/nginx-fix-pidfile.patch
>> Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/nginx/+bug/1581864
>> Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=876365
>>
>> Upstream bug: (deferred fix)
>> https://trac.nginx.org/nginx/ticket/1897?cversion=0&cnum_hist=2
> 
> So, basically, upstream refused to fix that bug, because (re-ordered so
> that what I believe most important gets listed first):
> 
>   - the believe the Ubuntu patch (which this is based on) is incorrect:
>     "The patch suggested in the Ubuntu bugtracker to write pidfile in
>     the parent process looks wrong. In particular, it does so with the
>     wrong umask, which is set in the child process only."
> 
>   - they believe the synchronisation described in systemd's doc is too
>     complex:
>     "The patch to synchronize things as recommended bysystemd's
>     daemon(7) manpage [...] wasn't committed as the change is considered
>     too complex for the problem it solves"
> 
>   - they believe the message is innocuous (I suppose because systemd
>     will loop to TERM then KILL all proceses in a cgroup, thus actually
>     and eventually ending the nginx daemon).
> 
>   - a then-future version of systemd should include support for that
>     situation with new the new unit directive "Type=pid-file"
> 
>  - a few various lesser interesting (to us) reasons
> 
> However, there seems to be an easy workaround: complement the systemd
> unit with:
> 
>     ExecStartPost=/bin/sleep 0.1
> 
> (or even a bigger number if we want to support slow systems).
> 
> Given that the patch on the code was rejected by upstream, I am wary to
> bundle it in Buildroot, because we will supposedly have to carry it
> forever. In this respect, tweaking the ystemd unit seems a leser burden.
> 
> However, what about the systemd situation: is the reference
> Type=pid-file feautre now implemented? Do we have it in the version
> packaged in Buildroot?
> 
> If so, maybe we could then tweak the unit to make use of it, instead of
> the sleep workaround?

 Current git master still has it in TODO, so I guess not..

 I think there was one useful comment on the Ubuntu bug [1]:

>>>
As far as I can tell, this daemonization is not needed for the systemd service
use-case. The service should be Type=simple, and 'daemon off'. The standard file
handles get redirected by systemd anyway, and non-stop upgrade cannot be used in
this case either. (See:
http://nginx.org/en/docs/faq/daemon_master_process_off.html )
<<<

 So I rather think we should go for that solution.


 Regards,
 Arnout

[1] https://bugs.launchpad.net/ubuntu/+source/nginx/+bug/1581864/comments/7



> 
> Regards,
> Yann E. MORIN.
> 
>> Signed-off-by: Matthew Weber <matthew.weber@collins.com>
>> ---
>>  .../0010-Fix-NGINX-pidfile-handling.patch     | 105 ++++++++++++++++++
>>  1 file changed, 105 insertions(+)
>>  create mode 100644 package/nginx/0010-Fix-NGINX-pidfile-handling.patch
>>
>> diff --git a/package/nginx/0010-Fix-NGINX-pidfile-handling.patch b/package/nginx/0010-Fix-NGINX-pidfile-handling.patch
>> new file mode 100644
>> index 0000000000..9a50118434
>> --- /dev/null
>> +++ b/package/nginx/0010-Fix-NGINX-pidfile-handling.patch
>> @@ -0,0 +1,105 @@
>> +From 2491379370059c7b9a2b956a49c90a9de55f5dcb Mon Sep 17 00:00:00 2001
>> +From: Tj <ubuntu@iam.tj>
>> +Date: Fri, 21 May 2021 09:35:53 -0500
>> +Subject: [PATCH] Fix NGINX pidfile handling
>> +
>> +Based on https://git.launchpad.net/ubuntu/+source/nginx/plain/debian/patches/nginx-fix-pidfile.patch
>> +Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/nginx/+bug/1581864
>> +Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=876365
>> +Last-Update: 2020-06-24
>> +
>> +Upstream bug: (deferred fix)
>> +https://trac.nginx.org/nginx/ticket/1897?cversion=0&cnum_hist=2
>> +
>> +Signed-off-by: Tj <ubuntu@iam.tj>
>> +Signed-off-by: Matthew Weber <matthew.weber@collins.com>
>> +---
>> + src/core/nginx.c         | 24 +++++++++++++++++++++---
>> + src/os/unix/ngx_daemon.c |  8 ++++++--
>> + 2 files changed, 27 insertions(+), 5 deletions(-)
>> +
>> +diff --git a/src/core/nginx.c b/src/core/nginx.c
>> +index 9fcb0eb..083eba1 100644
>> +--- a/src/core/nginx.c
>> ++++ b/src/core/nginx.c
>> +@@ -338,14 +338,21 @@ main(int argc, char *const *argv)
>> +         ngx_process = NGX_PROCESS_MASTER;
>> +     }
>> + 
>> ++    /* tell-tale to detect if this is parent or child process */
>> ++    ngx_int_t child_pid = NGX_BUSY;
>> ++
>> + #if !(NGX_WIN32)
>> + 
>> +     if (ngx_init_signals(cycle->log) != NGX_OK) {
>> +         return 1;
>> +     }
>> + 
>> ++    /* tell-tale that this code has been executed */
>> ++    child_pid--;
>> ++
>> +     if (!ngx_inherited && ccf->daemon) {
>> +-        if (ngx_daemon(cycle->log) != NGX_OK) {
>> ++        child_pid = ngx_daemon(cycle->log);
>> ++        if (child_pid == NGX_ERROR) {
>> +             return 1;
>> +         }
>> + 
>> +@@ -358,8 +365,19 @@ main(int argc, char *const *argv)
>> + 
>> + #endif
>> + 
>> +-    if (ngx_create_pidfile(&ccf->pid, cycle->log) != NGX_OK) {
>> +-        return 1;
>> ++    /* If ngx_daemon() returned the child's PID in the parent process
>> ++     * after the fork() set ngx_pid to the child_pid, which gets
>> ++     * written to the PID file, then exit.
>> ++     * For NGX_WIN32 always write the PID file
>> ++     * For others, only write it from the parent process */
>> ++    if (child_pid < NGX_OK || child_pid > NGX_OK) {
>> ++	ngx_pid = child_pid > NGX_OK ? child_pid : ngx_pid;
>> ++        if (ngx_create_pidfile(&ccf->pid, cycle->log) != NGX_OK) {
>> ++            return 1;
>> ++	}
>> ++    }
>> ++    if (child_pid > NGX_OK) {
>> ++        exit(0);
>> +     }
>> + 
>> +     if (ngx_log_redirect_stderr(cycle) != NGX_OK) {
>> +diff --git a/src/os/unix/ngx_daemon.c b/src/os/unix/ngx_daemon.c
>> +index 385c49b..3719854 100644
>> +--- a/src/os/unix/ngx_daemon.c
>> ++++ b/src/os/unix/ngx_daemon.c
>> +@@ -7,14 +7,17 @@
>> + 
>> + #include <ngx_config.h>
>> + #include <ngx_core.h>
>> ++#include <unistd.h>
>> + 
>> + 
>> + ngx_int_t
>> + ngx_daemon(ngx_log_t *log)
>> + {
>> +     int  fd;
>> ++    /* retain the return value for passing back to caller */
>> ++    pid_t pid_child = fork();
>> + 
>> +-    switch (fork()) {
>> ++    switch (pid_child) {
>> +     case -1:
>> +         ngx_log_error(NGX_LOG_EMERG, log, ngx_errno, "fork() failed");
>> +         return NGX_ERROR;
>> +@@ -23,7 +26,8 @@ ngx_daemon(ngx_log_t *log)
>> +         break;
>> + 
>> +     default:
>> +-        exit(0);
>> ++        /* let caller do the exit() */
>> ++        return pid_child;
>> +     }
>> + 
>> +     ngx_parent = ngx_pid;
>> +-- 
>> +2.17.1
>> +
>> -- 
>> 2.17.1
>>
>> _______________________________________________
>> buildroot mailing list
>> buildroot@busybox.net
>> http://lists.busybox.net/mailman/listinfo/buildroot
>

Arnout, All,

On 2021-05-27 22:49 +0200, Arnout Vandecappelle spake thusly:
> On 26/05/2021 22:48, Yann E. MORIN wrote:
> > On 2021-05-21 09:41 -0500, Matthew Weber via buildroot spake thusly:
> >> Upstream bug: (deferred fix)
> >> https://trac.nginx.org/nginx/ticket/1897?cversion=0&cnum_hist=2
> > So, basically, upstream refused to fix that bug [...]
[--SNIP--]
> > If so, maybe we could then tweak the unit to make use of it, instead of
> > the sleep workaround?
>  I think there was one useful comment on the Ubuntu bug [1]:
> >>>
> As far as I can tell, this daemonization is not needed for the systemd service
> use-case. The service should be Type=simple, and 'daemon off'. The standard file
> handles get redirected by systemd anyway, and non-stop upgrade cannot be used in
> this case either. (See:
> http://nginx.org/en/docs/faq/daemon_master_process_off.html )
> <<<
>  So I rather think we should go for that solution.

Indeed, that seems an even better solution, and probably an actual fix.

Regards,
Yann E. MORIN.

On 28/05/2021 13:39, Yann E. MORIN wrote:
> Arnout, All,
> 
> On 2021-05-27 22:49 +0200, Arnout Vandecappelle spake thusly:
>> On 26/05/2021 22:48, Yann E. MORIN wrote:
>>> On 2021-05-21 09:41 -0500, Matthew Weber via buildroot spake thusly:
>>>> Upstream bug: (deferred fix)
>>>> https://trac.nginx.org/nginx/ticket/1897?cversion=0&cnum_hist=2
>>> So, basically, upstream refused to fix that bug [...]
> [--SNIP--]
>>> If so, maybe we could then tweak the unit to make use of it, instead of
>>> the sleep workaround?
>>  I think there was one useful comment on the Ubuntu bug [1]:
>>>>>
>> As far as I can tell, this daemonization is not needed for the systemd service
>> use-case. The service should be Type=simple, and 'daemon off'. The standard file
>> handles get redirected by systemd anyway, and non-stop upgrade cannot be used in
>> this case either. (See:
>> http://nginx.org/en/docs/faq/daemon_master_process_off.html )
>> <<<
>>  So I rather think we should go for that solution.
> 
> Indeed, that seems an even better solution, and probably an actual fix.

 Unfortunately, not so easy in practice. "daemon off" means a change in the
configuration file, which is typically going to be modified by the user. Also,
it's something that comes from nginx itself, so we'd have to patch the one that
gets installed by nginx (and that one will probably be overwritten anyway in an
overlay).

 Also, daemon off should only be done in systemd; sysvinit still wants nginx to
daemonize! So it's conditional patching, and we don't like that... Well, I guess
it can be done post-install with a sed script that inserts an aditional line at
the beginning of the file.


 Regards,
 Arnout

Arnout, All,

On 2021-05-28 13:51 +0200, Arnout Vandecappelle spake thusly:
> On 28/05/2021 13:39, Yann E. MORIN wrote:
> > On 2021-05-27 22:49 +0200, Arnout Vandecappelle spake thusly:
> >> On 26/05/2021 22:48, Yann E. MORIN wrote:
> >>> On 2021-05-21 09:41 -0500, Matthew Weber via buildroot spake thusly:
> >>>> Upstream bug: (deferred fix)
> >>>> https://trac.nginx.org/nginx/ticket/1897?cversion=0&cnum_hist=2
> >>> So, basically, upstream refused to fix that bug [...]
> > [--SNIP--]
> >>> If so, maybe we could then tweak the unit to make use of it, instead of
> >>> the sleep workaround?
> >>  I think there was one useful comment on the Ubuntu bug [1]:
> >>>>>
> >> As far as I can tell, this daemonization is not needed for the systemd service
> >> use-case. The service should be Type=simple, and 'daemon off'. The standard file
> >> handles get redirected by systemd anyway, and non-stop upgrade cannot be used in
> >> this case either. (See:
> >> http://nginx.org/en/docs/faq/daemon_master_process_off.html )
> >> <<<
> >>  So I rather think we should go for that solution.
> > 
> > Indeed, that seems an even better solution, and probably an actual fix.
> 
>  Unfortunately, not so easy in practice. "daemon off" means a change in the
> configuration file, which is typically going to be modified by the user. Also,
> it's something that comes from nginx itself, so we'd have to patch the one that
> gets installed by nginx (and that one will probably be overwritten anyway in an
> overlay).
> 
>  Also, daemon off should only be done in systemd; sysvinit still wants nginx to
> daemonize! So it's conditional patching, and we don't like that... Well, I guess
> it can be done post-install with a sed script that inserts an aditional line at
> the beginning of the file.

Well, except for the overlay issue, it seems to be pretty easy to do;

    diff --git a/package/nginx/nginx.mk b/package/nginx/nginx.mk
    index e93e802fd3..411d686cdd 100644
    --- a/package/nginx/nginx.mk
    +++ b/package/nginx/nginx.mk
    @@ -314,6 +314,10 @@ endef
     define NGINX_INSTALL_INIT_SYSTEMD
     	$(INSTALL) -D -m 0644 package/nginx/nginx.service \
     		$(TARGET_DIR)/usr/lib/systemd/system/nginx.service
    +	mkdir -p $(TARGET_DIR)/etc/systemd/system/nginx.d
    +	$(SED) '1idaemon off;' $(TARGET_DIR)/etc/nginx.conf
    +	printf '[Service]\nType=simple\n' \
    +		>$(TARGET_DIR)/etc/systemd/system/nginx.d/buildroot-no-daemon.conf
     endef
     
     define NGINX_INSTALL_INIT_SYSV

But of course, is a user provides their daemon-using config file, they'd
be out of luck...

In the end, maybe the sleep-based workaround is just good-enough?

Regards,
Yann E. MORIN.

On 28/05/2021 15:12, Yann E. MORIN wrote:
> Arnout, All,
> 
> On 2021-05-28 13:51 +0200, Arnout Vandecappelle spake thusly:
>> On 28/05/2021 13:39, Yann E. MORIN wrote:
>>> On 2021-05-27 22:49 +0200, Arnout Vandecappelle spake thusly:
>>>> On 26/05/2021 22:48, Yann E. MORIN wrote:
>>>>> On 2021-05-21 09:41 -0500, Matthew Weber via buildroot spake thusly:
>>>>>> Upstream bug: (deferred fix)
>>>>>> https://trac.nginx.org/nginx/ticket/1897?cversion=0&cnum_hist=2
>>>>> So, basically, upstream refused to fix that bug [...]
>>> [--SNIP--]
>>>>> If so, maybe we could then tweak the unit to make use of it, instead of
>>>>> the sleep workaround?
>>>>  I think there was one useful comment on the Ubuntu bug [1]:
>>>>>>>
>>>> As far as I can tell, this daemonization is not needed for the systemd service
>>>> use-case. The service should be Type=simple, and 'daemon off'. The standard file
>>>> handles get redirected by systemd anyway, and non-stop upgrade cannot be used in
>>>> this case either. (See:
>>>> http://nginx.org/en/docs/faq/daemon_master_process_off.html )
>>>> <<<
>>>>  So I rather think we should go for that solution.
>>>
>>> Indeed, that seems an even better solution, and probably an actual fix.
>>
>>  Unfortunately, not so easy in practice. "daemon off" means a change in the
>> configuration file, which is typically going to be modified by the user. Also,
>> it's something that comes from nginx itself, so we'd have to patch the one that
>> gets installed by nginx (and that one will probably be overwritten anyway in an
>> overlay).
>>
>>  Also, daemon off should only be done in systemd; sysvinit still wants nginx to
>> daemonize! So it's conditional patching, and we don't like that... Well, I guess
>> it can be done post-install with a sed script that inserts an aditional line at
>> the beginning of the file.
> 
> Well, except for the overlay issue, it seems to be pretty easy to do;

 I only arrived at the sed idea when I had already written all the rest and
didn't bother to remove my previous thinking.

> 
>     diff --git a/package/nginx/nginx.mk b/package/nginx/nginx.mk
>     index e93e802fd3..411d686cdd 100644
>     --- a/package/nginx/nginx.mk
>     +++ b/package/nginx/nginx.mk
>     @@ -314,6 +314,10 @@ endef
>      define NGINX_INSTALL_INIT_SYSTEMD
>      	$(INSTALL) -D -m 0644 package/nginx/nginx.service \
>      		$(TARGET_DIR)/usr/lib/systemd/system/nginx.service
>     +	mkdir -p $(TARGET_DIR)/etc/systemd/system/nginx.d
>     +	$(SED) '1idaemon off;' $(TARGET_DIR)/etc/nginx.conf
>     +	printf '[Service]\nType=simple\n' \
>     +		>$(TARGET_DIR)/etc/systemd/system/nginx.d/buildroot-no-daemon.conf

 The service file is ours, so we can simply edit it directly.

>      endef
>      
>      define NGINX_INSTALL_INIT_SYSV
> 
> But of course, is a user provides their daemon-using config file, they'd
> be out of luck...
> 
> In the end, maybe the sleep-based workaround is just good-enough?

 I think if the daemon off works, it's a better solution.

 For the user-provided config file: we can make it explicit in the release
notes. The effect will be that systemd detects that nginx isn't forking and
kills it off after a few seconds, so the user will notice pretty quickly.


 I'm thinking (adding Peter in Cc) that we should maybe do better on the release
notes. Yocto (ever our example) puts a (now very long) "Migrating to a Newer
Yocto Project Release" [1] directly in their documentation with a description of
every release. We do have an upgrading section, but maybe we should put any
caveats directly in there instead of trying to compile them at release time in
the CHANGES file.

 Regards,
 Arnout

[1] https://www.yoctoproject.org/docs/2.2/ref-manual/ref-manual.html#migration

>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

Hi,

 >  Unfortunately, not so easy in practice. "daemon off" means a change in the
 > configuration file, which is typically going to be modified by the user. Also,
 > it's something that comes from nginx itself, so we'd have to patch the one that
 > gets installed by nginx (and that one will probably be overwritten anyway in an
 > overlay).

Notice that you can override the configuration on the command line with
the -g option (-g 'daemon off;') like it is typically done in
Dockerfiles:

https://github.com/nginxinc/docker-nginx/blob/master/Dockerfile-debian.template#L106

>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

Hi,

 >  I'm thinking (adding Peter in Cc) that we should maybe do better on the release
 > notes. Yocto (ever our example) puts a (now very long) "Migrating to a Newer
 > Yocto Project Release" [1] directly in their documentation with a description of
 > every release. We do have an upgrading section, but maybe we should put any
 > caveats directly in there instead of trying to compile them at release time in
 > the CHANGES file.

Yes, I agree. We already have a section in the manual for it:

https://github.com/nginxinc/docker-nginx/blob/master/Dockerfile-debian.template#L106

But very little content :/ Patches accepted!

On 28/05/2021 17:46, Peter Korsgaard wrote:
>>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:
> 
> Hi,
> 
>  >  Unfortunately, not so easy in practice. "daemon off" means a change in the
>  > configuration file, which is typically going to be modified by the user. Also,
>  > it's something that comes from nginx itself, so we'd have to patch the one that
>  > gets installed by nginx (and that one will probably be overwritten anyway in an
>  > overlay).
> 
> Notice that you can override the configuration on the command line with
> the -g option (-g 'daemon off;')

 I didn't realize that was possible! Then we can do it from the service file
directly, and it becomes trivial. Matt?

 Regards,
 Arnout

> like it is typically done in
> Dockerfiles:
> 
> https://github.com/nginxinc/docker-nginx/blob/master/Dockerfile-debian.template#L106
>

On 28/05/2021 17:48, Peter Korsgaard wrote:
>>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:
> 
> Hi,
> 
>  >  I'm thinking (adding Peter in Cc) that we should maybe do better on the release
>  > notes. Yocto (ever our example) puts a (now very long) "Migrating to a Newer
>  > Yocto Project Release" [1] directly in their documentation with a description of
>  > every release. We do have an upgrading section, but maybe we should put any
>  > caveats directly in there instead of trying to compile them at release time in
>  > the CHANGES file.
> 
> Yes, I agree. We already have a section in the manual for it:
> 
> https://github.com/nginxinc/docker-nginx/blob/master/Dockerfile-debian.template#L106

 I think you meant

https://buildroot.org/downloads/manual/manual.html#migrating-from-ol-versions

:-)


> But very little content :/ Patches accepted!

 What I meant is: we should require an update of that section whenever there's a
potentially breaking change. From 2021.05-rc1, for example:

        FORTIFY_SOURCE, PIC/PIE, RELRO and SSP security hardening
        options are now enabled by default.

The change of the default could have updated that section in the same patch.


 Regards,
 Arnout

>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

Hi,

 >> Yes, I agree. We already have a section in the manual for it:
 >> 
 >> https://github.com/nginxinc/docker-nginx/blob/master/Dockerfile-debian.template#L106

 >  I think you meant

 > https://buildroot.org/downloads/manual/manual.html#migrating-from-ol-versions

Hah, indeed ;)


 >> But very little content :/ Patches accepted!

 >  What I meant is: we should require an update of that section whenever there's a
 > potentially breaking change. From 2021.05-rc1, for example:

 >         FORTIFY_SOURCE, PIC/PIE, RELRO and SSP security hardening
 >         options are now enabled by default.

 > The change of the default could have updated that section in the same patch.

Yes, I agree. I will try to remind contributors in the future.

Arnout, Peter, Matthew, All,

On 2021-05-28 18:05 +0200, Arnout Vandecappelle spake thusly:
> On 28/05/2021 17:46, Peter Korsgaard wrote:
> > Notice that you can override the configuration on the command line with
> > the -g option (-g 'daemon off;')
>  I didn't realize that was possible! Then we can do it from the service file
> directly, and it becomes trivial. Matt?

Ah indeed, that really sounds like the proper workaround/solution/fix.

Regards,
Yann E. MORIN.