bug#31498: [PATCH] Notify systemd in daemon-initialized

bug#31498: [PATCH] Notify systemd in daemon-initialized

[Lucas Werkmeister]

With --[bg-]daemon and Type=forking, systemd will only consider the
daemon to have fully started up once the original process exits, and
will wait until then to start units depending on the Emacs service. To
get the same functionality with --fg-daemon, use Type=notify instead of
Type=simple and explicitly send a readiness notification to systemd at
the point where the forked process would in --bg-daemon mode notify its
parent process and cause it to exit.
* etc/emacs.service: update Type
* src/emacs.c (daemon-initialized): call sd_notify()
---
 etc/emacs.service | 2 +-
 src/emacs.c       | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/etc/emacs.service b/etc/emacs.service
index b29177b120..dbcb6bc301 100644
--- a/etc/emacs.service
+++ b/etc/emacs.service
@@ -7,7 +7,7 @@ Description=Emacs text editor
 Documentation=info:emacs man:emacs(1) https://gnu.org/software/emacs/
 
 [Service]
-Type=simple
+Type=notify
 ExecStart=emacs --fg-daemon
 ExecStop=emacsclient --eval "(kill-emacs)"
 Environment=SSH_AUTH_SOCK=%t/keyring/ssh
diff --git a/src/emacs.c b/src/emacs.c
index f25e612664..8718b301a4 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -2477,6 +2477,13 @@ from the parent process and its tty file descriptors.  */)
     error ("This function can only be called after loading the init files");
 #ifndef WINDOWSNT
 
+  if (daemon_type == 1)
+    {
+#ifdef HAVE_LIBSYSTEMD
+      sd_notify(0, "READY=1");
+#endif /* HAVE_LIBSYSTEMD */
+    }
+
   if (daemon_type == 2)
     {
       int nfd;
-- 
2.17.0

bug#31498: [PATCH] Notify systemd in daemon-initialized

[Noam Postavsky]

severity 31498 wishlist
severity 31521 wishlist
quit


Thanks, the patch seems straightforward enough.  Just a few minor
comments about formatting.  Have you assigned copyright for Emacs?  The
patch is small enough to go in anyway, it would just have to be marked.

Lucas Werkmeister <mail@lucaswerkmeister.de> writes:

> With --[bg-]daemon and Type=forking, systemd will only consider the
> daemon to have fully started up once the original process exits, and
> will wait until then to start units depending on the Emacs service. To
                                                                     ^
                                                                     double space
> get the same functionality with --fg-daemon, use Type=notify instead of
> Type=simple and explicitly send a readiness notification to systemd at
> the point where the forked process would in --bg-daemon mode notify its
> parent process and cause it to exit.
> * etc/emacs.service: update Type

The ChangeLog entries should be captilalized and end with a period:
"Update Type."

> * src/emacs.c (daemon-initialized): call sd_notify()

Same here.

I think it should have a NEWS entry mentioning that Emacs now supports
this systemd notification feature.  I would probably go ahead and
combine with #31521, since it's using the same feature.

bug#31498: [PATCH] Notify systemd in daemon-initialized

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Thu, 07 Jun 2018 09:08:51 -0400
> Cc: 31498@debbugs.gnu.org
> 
> Have you assigned copyright for Emacs?

I don't see an assignment for Lucas Werkmeister on file.

bug#31498: [PATCH] Notify systemd in daemon-initialized

[Lucas Werkmeister]

Thanks for the review! Replies inline.

On 07.06.2018 15:08, Noam Postavsky wrote:
> severity 31498 wishlist
> severity 31521 wishlist
> quit
> 
> 
> Thanks, the patch seems straightforward enough.  Just a few minor
> comments about formatting.  Have you assigned copyright for Emacs?  The
> patch is small enough to go in anyway, it would just have to be marked.

I haven’t assigned copyright yet, but I’d be happy to do so – I’ve tried
to get the process started on emacs-devel.

> 
> Lucas Werkmeister <mail@lucaswerkmeister.de> writes:
> 
>> With --[bg-]daemon and Type=forking, systemd will only consider the
>> daemon to have fully started up once the original process exits, and
>> will wait until then to start units depending on the Emacs service. To
>                                                                      ^
>                                                                      double space

ack

>> get the same functionality with --fg-daemon, use Type=notify instead of
>> Type=simple and explicitly send a readiness notification to systemd at
>> the point where the forked process would in --bg-daemon mode notify its
>> parent process and cause it to exit.
>> * etc/emacs.service: update Type
> 
> The ChangeLog entries should be captilalized and end with a period:
> "Update Type."
> 
>> * src/emacs.c (daemon-initialized): call sd_notify()
> 
> Same here.

2× ack

> 
> I think it should have a NEWS entry mentioning that Emacs now supports
> this systemd notification feature.  I would probably go ahead and
> combine with #31521, since it's using the same feature.
> 

Do you mean, announce both in this patch, or completely merge that patch
into this one?

Once I’ve fixed the problems locally, what’s the best way to re-roll the
patch in this bug tracking system? Should I try to configure `git
send-email` to reply to the right email (and with the right bug in CC),
or perhaps use `git format-patch` and add the patch file as a regular
attachment in a manual response?

bug#31498: [PATCH] Notify systemd in daemon-initialized

[Noam Postavsky]

Lucas Werkmeister <mail@lucaswerkmeister.de> writes:

> I haven’t assigned copyright yet, but I’d be happy to do so – I’ve tried
> to get the process started on emacs-devel.

Cool, since this patch will be destined for Emacs 27, there's not much
urgency, so I'll wait for that to resolve before pushing.

>> I think it should have a NEWS entry mentioning that Emacs now supports
>> this systemd notification feature.  I would probably go ahead and
>> combine with #31521, since it's using the same feature.
>> 
>
> Do you mean, announce both in this patch, or completely merge that patch
> into this one?

The latter (maybe it would have been clearer to put those two sentences
in separate paragraphs).

> Once I’ve fixed the problems locally, what’s the best way to re-roll the
> patch in this bug tracking system? Should I try to configure `git
> send-email` to reply to the right email (and with the right bug in CC),
> or perhaps use `git format-patch` and add the patch file as a regular
> attachment in a manual response?

Either way is fine I guess, though I've never tried 'git send-email'
myself.

bug#31498: [PATCH] Notify systemd in daemon-initialized and kill-emacs

[Lucas Werkmeister]

With --[bg-]daemon and Type=forking, systemd will only consider the
daemon to have fully started up once the original process exits, and
will wait until then to start units depending on the Emacs service.  To
get the same functionality with --fg-daemon, use Type=notify instead of
Type=simple and explicitly send a readiness notification to systemd at
the point where the forked process would in --bg-daemon mode notify its
parent process and cause it to exit. Similarly, notify systemd at the
beginning of the shutdown process as well. (Both of these calls are
successful no-ops if emacs was not started by systemd.)
* etc/emacs.service: Update Type.
* src/emacs.c (daemon-initialized): Call sd_notify().
* src/emacs.c (kill-emacs): Call sd_notify().
---
 etc/emacs.service |  2 +-
 src/emacs.c       | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/etc/emacs.service b/etc/emacs.service
index b29177b120..dbcb6bc301 100644
--- a/etc/emacs.service
+++ b/etc/emacs.service
@@ -7,7 +7,7 @@ Description=Emacs text editor
 Documentation=info:emacs man:emacs(1) https://gnu.org/software/emacs/
 
 [Service]
-Type=simple
+Type=notify
 ExecStart=emacs --fg-daemon
 ExecStop=emacsclient --eval "(kill-emacs)"
 Environment=SSH_AUTH_SOCK=%t/keyring/ssh
diff --git a/src/emacs.c b/src/emacs.c
index 8633e934e5..cde20fc096 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -2017,6 +2017,10 @@ all of which are called before Emacs is actually killed.  */
 {
   int exit_code;
 
+#ifdef HAVE_LIBSYSTEMD
+  sd_notify(0, "STOPPING=1");
+#endif /* HAVE_LIBSYSTEMD */
+
   /* Fsignal calls emacs_abort () if it sees that waiting_for_input is
      set.  */
   waiting_for_input = 0;
@@ -2477,6 +2481,13 @@ from the parent process and its tty file descriptors.  */)
     error ("This function can only be called after loading the init files");
 #ifndef WINDOWSNT
 
+  if (daemon_type == 1)
+    {
+#ifdef HAVE_LIBSYSTEMD
+      sd_notify(0, "READY=1");
+#endif /* HAVE_LIBSYSTEMD */
+    }
+
   if (daemon_type == 2)
     {
       int nfd;
-- 
2.17.1

bug#31498: (no subject)

[Lucas Werkmeister]

[-- Attachment #1: Type: text/plain, Size: 90 bytes --]

sorry, forgot the double-space after periods after editing the commit
message, rerolling


[-- Attachment #2: Type: text/html, Size: 274 bytes --]

bug#31498: [PATCH] Notify systemd in daemon-initialized and kill-emacs

[Lucas Werkmeister]

With --[bg-]daemon and Type=forking, systemd will only consider the
daemon to have fully started up once the original process exits, and
will wait until then to start units depending on the Emacs service.  To
get the same functionality with --fg-daemon, use Type=notify instead of
Type=simple and explicitly send a readiness notification to systemd at
the point where the forked process would in --bg-daemon mode notify its
parent process and cause it to exit.  Similarly, notify systemd at the
beginning of the shutdown process as well.  (Both of these calls are
successful no-ops if emacs was not started by systemd.)
* etc/emacs.service: Update Type.
* src/emacs.c (daemon-initialized): Call sd_notify().
* src/emacs.c (kill-emacs): Call sd_notify().
---
 etc/emacs.service |  2 +-
 src/emacs.c       | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/etc/emacs.service b/etc/emacs.service
index b29177b120..dbcb6bc301 100644
--- a/etc/emacs.service
+++ b/etc/emacs.service
@@ -7,7 +7,7 @@ Description=Emacs text editor
 Documentation=info:emacs man:emacs(1) https://gnu.org/software/emacs/
 
 [Service]
-Type=simple
+Type=notify
 ExecStart=emacs --fg-daemon
 ExecStop=emacsclient --eval "(kill-emacs)"
 Environment=SSH_AUTH_SOCK=%t/keyring/ssh
diff --git a/src/emacs.c b/src/emacs.c
index 8633e934e5..cde20fc096 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -2017,6 +2017,10 @@ all of which are called before Emacs is actually killed.  */
 {
   int exit_code;
 
+#ifdef HAVE_LIBSYSTEMD
+  sd_notify(0, "STOPPING=1");
+#endif /* HAVE_LIBSYSTEMD */
+
   /* Fsignal calls emacs_abort () if it sees that waiting_for_input is
      set.  */
   waiting_for_input = 0;
@@ -2477,6 +2481,13 @@ from the parent process and its tty file descriptors.  */)
     error ("This function can only be called after loading the init files");
 #ifndef WINDOWSNT
 
+  if (daemon_type == 1)
+    {
+#ifdef HAVE_LIBSYSTEMD
+      sd_notify(0, "READY=1");
+#endif /* HAVE_LIBSYSTEMD */
+    }
+
   if (daemon_type == 2)
     {
       int nfd;
-- 
2.17.1

bug#31498: [PATCH] Notify systemd in daemon-initialized and kill-emacs

[Noam Postavsky]

> >> I think it should have a NEWS entry mentioning that Emacs now supports
> >> this systemd notification feature.  I would probably go ahead and
> >> combine with #31521, since it's using the same feature.
> >> 
> >
> > Do you mean, announce both in this patch, or completely merge that patch
> > into this one?

> The latter (maybe it would have been clearer to put those two sentences
> in separate paragraphs).

I think I still managed to be unclear about this; I meant you should
both announce the change in etc/NEWS, and merge the patches.

Lucas Werkmeister <mail@lucaswerkmeister.de> writes:

> With --[bg-]daemon and Type=forking, systemd will only consider the
> daemon to have fully started up once the original process exits, and
> will wait until then to start units depending on the Emacs service.  To
> get the same functionality with --fg-daemon, use Type=notify instead of
> Type=simple and explicitly send a readiness notification to systemd at
> the point where the forked process would in --bg-daemon mode notify its
> parent process and cause it to exit.  Similarly, notify systemd at the
> beginning of the shutdown process as well.  (Both of these calls are
> successful no-ops if emacs was not started by systemd.)
> * etc/emacs.service: Update Type.
> * src/emacs.c (daemon-initialized): Call sd_notify().
> * src/emacs.c (kill-emacs): Call sd_notify().

Thanks, I think I still managed to be unclear about the 

bug#31498: [PATCH] Notify systemd in daemon-initialized and kill-emacs

[Lucas Werkmeister]

On 09.06.2018 14:29, Noam Postavsky wrote:
> 
> I think I still managed to be unclear about this; I meant you should
> both announce the change in etc/NEWS, and merge the patches.

Sorry, no, that’s not your fault, I just forgot :) will reroll

bug#31498: [PATCH] Notify systemd in daemon-initialized and kill-emacs

[Noam Postavsky]

Noam Postavsky <npostavs@gmail.com> writes:

> Lucas Werkmeister <mail@lucaswerkmeister.de> writes:
>
>> * etc/emacs.service: Update Type.
>> * src/emacs.c (daemon-initialized): Call sd_notify().
>> * src/emacs.c (kill-emacs): Call sd_notify().

Oops, hit send too early.  I wanted to add, that you don't need to
repeat the same message for the last two ChangeLog entries, and you
should use the "Conditional Changes" syntax [1], like so:

* src/emacs.c (daemon-initialized) [HAVE_LIBSYSTEMD]:
* src/emacs.c (kill-emacs) [HAVE_LIBSYSTEMD]: Call sd_notify().

[1]: https://www.gnu.org/prep/standards/html_node/Conditional-Changes.html

PS I was wrong about 'git send-email' being just as good as replying
with attachment.  The bug tracker adds a "bug#xxxxx:" prefix, so I end
up getting duplicate emails.  Next time someone asks I'll know what to
answer :)

bug#31498: [PATCH] Notify systemd in daemon-initialized and kill-emacs

[Lucas Werkmeister]

> PS I was wrong about 'git send-email' being just as good as replying
> with attachment.  The bug tracker adds a "bug#xxxxx:" prefix, so I end
> up getting duplicate emails.  Next time someone asks I'll know what to
> answer :)
> 

Sorry, I think that was just because I manually added you to CC. I’ll
skip that for the next time and we’ll see if it’s better, okay? :)

bug#31498: [PATCH] Notify systemd in daemon-initialized and kill-emacs

[Lucas Werkmeister]

With --[bg-]daemon and Type=forking, systemd will only consider the
daemon to have fully started up once the original process exits, and
will wait until then to start units depending on the Emacs service.  To
get the same functionality with --fg-daemon, use Type=notify instead of
Type=simple and explicitly send a readiness notification to systemd at
the point where the forked process would in --bg-daemon mode notify its
parent process and cause it to exit.  Similarly, notify systemd at the
beginning of the shutdown process as well.  (Both of these calls are
successful no-ops if emacs was not started by systemd.)
* etc/emacs.service: Update Type.
* src/emacs.c (daemon-initialized) [HAVE_LIBSYSTEMD]:
* src/emacs.c (kill-emacs) [HAVE_LIBSYSTEMD]: Call sd_notify().
---
 etc/NEWS          |  8 ++++++++
 etc/emacs.service |  2 +-
 src/emacs.c       | 11 +++++++++++
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index 4ea3440754..ba693e7e52 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -74,6 +74,14 @@ work right without some adjustment:
 - you can use the new 'package-quickstart' so activation of packages does not
   need to pay attention to 'package-load-list' or 'package-user-dir' any more.
 
+---
+** Emacs now notifies systemd when startup finishes or shutdown begins.
+Units that are ordered after 'emacs.service' will only be started
+after Emacs has finished initialization and is ready for use.
+(If your Emacs is installed in a non-standard location and you copied the
+emacs.service file to eg ~/.config/systemd/user/, you will need to copy
+the new version of the file again.)
+
 \f
 * Changes in Emacs 27.1
 
diff --git a/etc/emacs.service b/etc/emacs.service
index b29177b120..dbcb6bc301 100644
--- a/etc/emacs.service
+++ b/etc/emacs.service
@@ -7,7 +7,7 @@ Description=Emacs text editor
 Documentation=info:emacs man:emacs(1) https://gnu.org/software/emacs/
 
 [Service]
-Type=simple
+Type=notify
 ExecStart=emacs --fg-daemon
 ExecStop=emacsclient --eval "(kill-emacs)"
 Environment=SSH_AUTH_SOCK=%t/keyring/ssh
diff --git a/src/emacs.c b/src/emacs.c
index 8633e934e5..cde20fc096 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -2017,6 +2017,10 @@ all of which are called before Emacs is actually killed.  */
 {
   int exit_code;
 
+#ifdef HAVE_LIBSYSTEMD
+  sd_notify(0, "STOPPING=1");
+#endif /* HAVE_LIBSYSTEMD */
+
   /* Fsignal calls emacs_abort () if it sees that waiting_for_input is
      set.  */
   waiting_for_input = 0;
@@ -2477,6 +2481,13 @@ from the parent process and its tty file descriptors.  */)
     error ("This function can only be called after loading the init files");
 #ifndef WINDOWSNT
 
+  if (daemon_type == 1)
+    {
+#ifdef HAVE_LIBSYSTEMD
+      sd_notify(0, "READY=1");
+#endif /* HAVE_LIBSYSTEMD */
+    }
+
   if (daemon_type == 2)
     {
       int nfd;
-- 
2.17.1

bug#31498: [PATCH] Notify systemd in daemon-initialized and kill-emacs

[Noam Postavsky]

Lucas Werkmeister <mail@lucaswerkmeister.de> writes:

>> PS I was wrong about 'git send-email' being just as good as replying
>> with attachment.  The bug tracker adds a "bug#xxxxx:" prefix, so I end
>> up getting duplicate emails.  Next time someone asks I'll know what to
>> answer :)
>> 
>
> Sorry, I think that was just because I manually added you to CC. I’ll
> skip that for the next time and we’ll see if it’s better, okay? :)

It's okay for this thread because I'm subscribed to the bug list, but in
general, if there were non-subscribed people discussing the bug then
they wouldn't have received the patch.


Anyway, the latest patch looks good, please ping the thread again when
the copyright stuff is done and I'll push to master.

bug#31498: [PATCH] Notify systemd in daemon-initialized and kill-emacs

[Lucas Werkmeister]

On 09.06.2018 16:05, Noam Postavsky wrote:> Anyway, the latest patch
looks good, please ping the thread again when
> the copyright stuff is done and I'll push to master.
> 

I think it’s done now, I just received a signed copy of the assignment
document (dated June 8th).

bug#31498: [PATCH] Notify systemd in daemon-initialized and kill-emacs

[Eli Zaretskii]

> From: Lucas Werkmeister <mail@lucaswerkmeister.de>
> Date: Wed, 13 Jun 2018 15:42:41 +0200
> 
> On 09.06.2018 16:05, Noam Postavsky wrote:> Anyway, the latest patch
> looks good, please ping the thread again when
> > the copyright stuff is done and I'll push to master.
> > 
> 
> I think it’s done now, I just received a signed copy of the assignment
> document (dated June 8th).

Your assignment is not yet on file, so let's please wait a few more
days for it to appear, which is a sign for us that the paperwork is
complete.

bug#31498: [PATCH] Notify systemd in daemon-initialized and, kill-emacs

[Lucas Werkmeister]

Any news on the copyright status? I haven’t received any further emails
from the FSF on this, so if there’s anything I still need to do, please
let me know :)

bug#31498: [PATCH] Notify systemd in daemon-initialized and, kill-emacs

[Eli Zaretskii]

> From: Lucas Werkmeister <mail@lucaswerkmeister.de>
> Date: Fri, 13 Jul 2018 00:20:15 +0200
> 
> Any news on the copyright status? I haven’t received any further emails
> from the FSF on this, so if there’s anything I still need to do, please
> let me know :)

Please ping the copyright clerk.  Your name is still not on file.

Sorry for the delay.

bug#31498: [PATCH] Notify systemd in daemon-initialized and, , kill-emacs

[Lucas Werkmeister]

I’ve been notified that my name should be on the file now.

bug#31498: [PATCH] Notify systemd in daemon-initialized and, , kill-emacs

[Eli Zaretskii]

> From: Lucas Werkmeister <mail@lucaswerkmeister.de>
> Date: Wed, 25 Jul 2018 14:03:08 +0200
> 
> I’ve been notified that my name should be on the file now.

Indeed, it is.

bug#31498: [PATCH] Notify systemd in daemon-initialized and, , kill-emacs

[Noam Postavsky]

merge 31498 31521
tags 31498 fixed
close 31498 27.1
quit

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lucas Werkmeister <mail@lucaswerkmeister.de>
>> Date: Wed, 25 Jul 2018 14:03:08 +0200
>> 
>> I’ve been notified that my name should be on the file now.
>
> Indeed, it is.

I've pushed to master.

[1: 19f5f7b19b]: 2018-07-25 21:07:03 -0400
  Notify systemd in daemon-initialized and kill-emacs (Bug#31498)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=19f5f7b19b0dcdae87476a3fd51c41f840b2b80f