From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.comp.gnu.guix.devel,gmane.emacs.devel Subject: Re: [Nicolas Graves] [PATCH v6 01/10] rde: emacs: Start emacs in --daemon mode, with shepherd and pid-file Date: Sun, 12 May 2024 09:29:45 +0300 Message-ID: <86bk5b1r1i.fsf@gnu.org> References: <20240410234923.29319-2-ngraves@ngraves.fr> <875xwotg35.fsf@trop.in> <87zfu0m9ps.fsf@ngraves.fr> <87jzl22u5w.fsf@gnu.org> <87frvp4a4u.fsf@ngraves.fr> <87msow5cm8.fsf@ngraves.fr> <87msow7xrs.fsf@ngraves.fr> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="36849"; mail-complaints-to="usenet@ciao.gmane.io" Cc: monnier@iro.umontreal.ca, ludo@gnu.org, emacs-devel@gnu.org, andrew@trop.in, guix-devel@gnu.org, bjorn.bidar@thaodan.de, rudi@constantly.at, felix.lechner@lease-up.com To: Nicolas Graves Original-X-From: guix-devel-bounces+gcggd-guix-devel=m.gmane-mx.org@gnu.org Sun May 12 08:30:54 2024 Return-path: Envelope-to: gcggd-guix-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1s62jV-0009Pq-Jg for gcggd-guix-devel@m.gmane-mx.org; Sun, 12 May 2024 08:30:53 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1s62ia-0004I5-Ow; Sun, 12 May 2024 02:29:56 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1s62iX-0004HX-Vt; Sun, 12 May 2024 02:29:54 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1s62iU-0004hK-QJ; Sun, 12 May 2024 02:29:50 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=MIME-version:References:Subject:In-Reply-To:To:From: Date; bh=Q5F9wWwZfP7xTkmpms6vzaP9NiSdmKXSkcbfcoP9IM8=; b=OidAqGBqA+q19aK/Q55f uBBss5Nh99KcLvlhzTDSXiYM1ocaRftX+6dXPaC1uEgoV0EeuaTJUZSqEu8GpiV63igEmaiWM3EtX cDTRDfbEU1jhQ8UH0hmrWxp/PqwMXxvSNqFHJ9+3Qf7TZOj5hrEFG8pEllgHieasNmXURihcOosVp LzEcW1Ttdjp292/zBnkBDrtnBxVkaWNsVdARKmF671L3nXP7N9c3wb5a85T3AAuVbT5WML9WXeurB aC06gJqnE8Y9X7fRc1xKPMRwKYSscxUXS0iPvst+cQd2GGUkCsGDYLRIfDHgsHxGhXTiTimjCCIvF Mc2Gy9B6BI6vDQ==; In-Reply-To: <87msow7xrs.fsf@ngraves.fr> (emacs-devel@gnu.org) X-BeenThere: guix-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane-mx.org@gnu.org Original-Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.comp.gnu.guix.devel:70196 gmane.emacs.devel:319176 Archived-At: > Cc: Ludovic Courtès , emacs-devel@gnu.org, > Andrew Tropin , 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." > > 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.