Re: [Nicolas Graves] [PATCH v6 01/10] rde: emacs: Start emacs in --daemon mode, with shepherd and pid-file

[Eli Zaretskii]

> Cc: Ludovic Courtès <ludo@gnu.org>, emacs-devel@gnu.org,
>  Andrew Tropin <andrew@trop.in>, guix-devel@gnu.org, bjorn.bidar@thaodan.de,
>  rudi@constantly.at, felix.lechner@lease-up.com
> Date: Sun, 12 May 2024 01:07:51 +0200
> From:  Nicolas Graves via "Emacs development discussions." 
> <emacs-devel@gnu.org>
> 
> A lightly cleaned-up version attached.

Thanks.  What was the motivation for this, again?

>  configure.ac     |  19 +------
>  lib/Makefile.in  |   2 +-
>  lib/gnulib.mk.in |   2 -
>  lib/sd-socket.c  | 137 +++++++++++++++++++++++++++++++++++++++++++++++
>  lib/sd-socket.h  |  57 ++++++++++++++++++++
>  msdos/sed1v2.inp |   3 --
>  src/Makefile.in  |   9 ++--
>  src/deps.mk      |   2 +-
>  src/emacs.c      |  50 ++++++++---------
>  9 files changed, 226 insertions(+), 55 deletions(-)
>  create mode 100644 lib/sd-socket.c
>  create mode 100644 lib/sd-socket.h

Your code is not from Gnulib, so it is wrong to place it in lib/.  It
should be in src/, and probably just an addition to sysdep.c.  Then
many of the changes to the build infrastructure will not be needed.

> -HAVE_LIBSYSTEMD=no
> -if test "${with_libsystemd}" = "yes" ; then
> -  dnl This code has been tested with libsystemd 222 and later.
> -  dnl FIXME: Find the earliest version number for which Emacs should work,
> -  dnl and change '222' to that number.
> -  EMACS_CHECK_MODULES([LIBSYSTEMD], [libsystemd >= 222],
> -    [HAVE_LIBSYSTEMD=yes], [HAVE_LIBSYSTEMD=no])
> -  if test "${HAVE_LIBSYSTEMD}" = "yes"; then
> -    AC_DEFINE([HAVE_LIBSYSTEMD], [1], [Define if using libsystemd.])
> -  fi
> -fi
> -
> -AC_SUBST([LIBSYSTEMD_LIBS])
> -AC_SUBST([LIBSYSTEMD_CFLAGS])

This test should be replaced by a test to determine whether the
systemd interface should be compiled into Emacs.  Not all of the
supported platform can use it, so we need to determine that at
configure time.

> -
>  HAVE_JSON=no
>  JSON_OBJ=
>  
> @@ -6652,7 +6636,7 @@ AC_DEFUN
>  optsep=
>  emacs_config_features=
>  for opt in ACL BE_APP CAIRO DBUS FREETYPE GCONF GIF GLIB GMP GNUTLS GPM 
> GSETTINGS \
> - HARFBUZZ IMAGEMAGICK JPEG JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 \
> + HARFBUZZ IMAGEMAGICK JPEG JSON LCMS2 LIBOTF LIBSELINUX LIBXML2 \
>   M17N_FLT MODULES NATIVE_COMP NOTIFY NS OLDXMENU PDUMPER PGTK PNG RSVG 
> SECCOMP \
>   SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS TREE_SITTER \
>   UNEXEC WEBP X11 XAW3D XDBE XFT XIM XINPUT2 XPM XWIDGETS X_TOOLKIT \
> @@ -6721,7 +6705,6 @@ AC_DEFUN
>    Does Emacs use -lm17n-flt?                              ${HAVE_M17N_FLT}
>    Does Emacs use -lotf?                                   ${HAVE_LIBOTF}
>    Does Emacs use -lxft?                                   ${HAVE_XFT}
> -  Does Emacs use -lsystemd?                               ${HAVE_LIBSYSTEMD}
>    Does Emacs use -ljansson?                               ${HAVE_JSON}
>    Does Emacs use -ltree-sitter?                           ${HAVE_TREE_SITTER}
>    Does Emacs use the GMP library?                         ${HAVE_GMP}

The above summary of the configuration should still call out the
result of the configure test for systemd interface, and I think the
list of features should include some string which tells us whether
systemd interface is supported.

> -#ifdef HAVE_LIBSYSTEMD
>        /* Read the number of sockets passed through by systemd.  */
> -      int systemd_socket = sd_listen_fds (1);
> -
> -      if (systemd_socket > 1)
> -        fputs (("\n"
> -             "Warning: systemd passed more than one socket to Emacs.\n"
> -             "Try 'Accept=false' in the Emacs socket unit file.\n"),
> -            stderr);
> -      else if (systemd_socket == 1
> -            && (0 < sd_is_socket (SD_LISTEN_FDS_START,
> -                                  AF_UNSPEC, SOCK_STREAM, 1)))
> -     sockfd = SD_LISTEN_FDS_START;
> -#endif /* HAVE_LIBSYSTEMD */
> +      const char *fds = getenv("LISTEN_FDS");
> +      if (fds) {
> +     int systemd_socket = strtol(fds, NULL, 0);
> +     if (systemd_socket > 1)
> +       fputs (("\n"
> +               "Warning: systemd passed more than one socket to Emacs.\n"
> +               "Try 'Accept=false' in the Emacs socket unit file.\n"),
> +              stderr);
> +     else if (systemd_socket == 1
> +              && (0 < sd_is_socket (SD_LISTEN_FDS_START, SOCK_STREAM, 1)))
> +       sockfd = SD_LISTEN_FDS_START;
> +      }

The new code to interface with systemd cannot be compiled
unconditionally, it should have some #ifdef condition, determined by
the configure script, because not all the platforms we support can use
systemd.

> @@ -2857,12 +2854,15 @@ DEFUN ("kill-emacs", Fkill_emacs, Skill_emacs, 0, 2, 
> "P",
>      }
>  #endif
>  
> -#ifdef HAVE_LIBSYSTEMD
>    /* Notify systemd we are shutting down, but only if we have notified
>       it about startup.  */
> -  if (daemon_type == -1)
> -    sd_notify(0, "STOPPING=1");
> -#endif /* HAVE_LIBSYSTEMD */
> +  if (daemon_type == -1){
> +    int r = sd_notify_stopping();
> +    if (r < 0) {
> +      fprintf(stderr, "Failed to report termination to $NOTIFY_SOCKET: 
> %s\n", strerror(-r));
> +      exit (EXIT_FAILURE);
> +    }
> +  }

Same here.

>    /* Fsignal calls emacs_abort () if it sees that waiting_for_input is
>       set.  */
> @@ -3382,9 +3382,11 @@ DEFUN ("daemon-initialized", Fdaemon_initialized, 
> Sdaemon_initialized, 0, 0, 0,
>  
>    if (daemon_type == 1)
>      {
> -#ifdef HAVE_LIBSYSTEMD
> -      sd_notify(0, "READY=1");
> -#endif /* HAVE_LIBSYSTEMD */
> +      int r = sd_notify_ready();
> +      if (r < 0) {
> +     fprintf(stderr, "Failed to notify readiness to $NOTIFY_SOCKET: %s\n", 
> strerror(-r));
> +     exit (EXIT_FAILURE);
> +      }
>      }

And here.

> > Seems to work properly on my side with the following patch, on Guix with
> > the shepherd service I sent a few weeks ago. The patch is actually
> > pretty simple.

How sure we are that the code you wrote will not need any significant
maintenance due to changes in how systemd works?  The significant
advantage of using libsystemd is that the systemd developers take care
of updating it as needed.  With these changes, that burden is now on
us.  I don't think we'd be happy maintaining system-level code that
have very little relevance to Emacs.

> > - The sd_notify code is taken from
> > https://www.freedesktop.org/software/systemd/man/devel/sd_notify.html#Notes

I'm not sure what would be the legal aspects of such code borrowing.