Re: [Bug-mcron] [PATCH] configure: Add '--with-sendmail' for (mcron config config-sendmail)

[宋文武]

Mathieu Lirzin <address@hidden> writes:

> Hello 宋文武, 
>
> address@hidden (宋文武) writes:
>
>> this patch add '--with-sendmail' option to the configure script.
>>
>> With it, I can set the sendmail command (config-sendmail) directly.
>>
>> Without it, the configure script fails to get a working sendmail command
>> due to missing 'ac_cv_prog_WHICH'...
>
> Indeed there is an issue here.
>
>> From f59066b5cc474fa5d531a1ee44baa46b728f4fbd Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?=E5=AE=8B=E6=96=87=E6=AD=A6?= <address@hidden>
>> Date: Mon, 1 Oct 2018 14:07:48 +0800
>> Subject: [PATCH] configure: Add '--with-sendmail' for (mcron config
>>  config-sendmail).
>>
>> * build-aux/guix.scm: Remove 'which' from native-inputs of the package.
>> * configure.ac: Add '--with-sendmail' option to set the command of sendmail
>> directly.
>> * src/mcron/redirect.scm (with-mail-out): Don't add USER to the command of
>> sendmail.
>
> I am not familiar with sendmail, so can you explain why USER has been
> removed from ‘with-mail-out’?

The idea is to configure the complete command with
'--with-sendmail=COMMAND', which avoids me a little trouble for write
the help string (otherwise, we have to say it's partially, the final
command will be "COMMAND USER", etc.).

Considered the 'sendmail' command from opensmtpd and the 'mail' command
from mailutils, they both can read recipients from the message header
(with the '-t' option), So I think nothing lose here.

>
> Additionally an entry in the NEWS files announcing the new configuration
> option and how/why to use it would be welcome.

Okay.

>
>> ---
>>  build-aux/guix.scm     |  3 +--
>>  configure.ac           | 29 +++++++++--------------------
>>  src/mcron/redirect.scm |  5 +----
>>  3 files changed, 11 insertions(+), 26 deletions(-)
> [...]
>> diff --git a/configure.ac b/configure.ac
>> index 0bb9262..e8a135a 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -55,26 +55,15 @@ GUILE_PROGS
>>  
>>  AM_MISSING_PROG(HELP2MAN, help2man, $missing_dir)
>>  
>> -# Now find a sendmail or equivalent.
>> -
>> -AC_CHECK_PROGS(SENDMAIL, sendmail)
>> -if test "x$ac_cv_prog_SENDMAIL" != "x"; then
>> -   AC_MSG_CHECKING(sendmail path and arguments)
>> -   ac_cv_prog_SENDMAIL="`$ac_cv_prog_WHICH sendmail` -FCronDaemon -odi -oem 
>> "
>> -dnl  -or0s"
>> -   AC_MSG_RESULT($ac_cv_prog_SENDMAIL)
>> -
>> -else
>> -   AC_CHECK_PROGS(SENDMAIL, mail)
>> -   if test "x$ac_cv_prog_SENDMAIL" != "x"; then
>> -      AC_MSG_CHECKING(mail path)
>> -      ac_cv_prog_SENDMAIL="`$ac_cv_prog_WHICH mail` -d "
>> -      AC_MSG_RESULT($ac_cv_prog_SENDMAIL)
>> -   else
>> -      AC_MSG_RESULT(No mail program found)
>> -   fi
>> -fi
>> -SENDMAIL=$ac_cv_prog_SENDMAIL
>> +AC_MSG_CHECKING([sendmail command])
>> +AC_ARG_WITH(sendmail,
>> +            AC_HELP_STRING([--with-sendmail=COMMAND],
>> +                           [command to read an email message from
>> +                           standard input, and send it]),
>> +              SENDMAIL=$withval,
>> +              SENDMAIL=["sendmail -t"])
>
> I guess you choose to remove the ‘AC_CHECK_PROGS’ to make ‘sendmail’ a
> loose dependency, meaning forcing ‘mcron’ to look at runtime in the PATH
> instead of having absolute file name set at configure time.  Am I
> correct?  If that's the case I think a comment regarding that fact would
> help.

Yes, and users can still specify the absolute file name for the command
when configure, it's more flexible.  I'll document it in the NEWS.

>
> Wouldn't the ‘-t’ option better located on the caller side (meaning in
> “src/mcron/redirect.scm”)?

I think letting users configure it should be more felxible (maybe there
is a MTA doesn't accept the '-t' option?).

>
> Nitpick: ‘AC_HELP_STRING’ is an obsolete macro which has been
> superseeded by ‘AS_HELP_STRING’.

Okay, thanks.

>
> Thank you for your patch.  Sorry for the delay.

Thank you for the review!


Here is the updated patch:

>From 8c17916b5949b7f273b63200538652d1b8337960 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E5=AE=8B=E6=96=87=E6=AD=A6?= <address@hidden>
Date: Mon, 1 Oct 2018 14:07:48 +0800
Subject: [PATCH] configure: Add '--with-sendmail' for (mcron config
 config-sendmail).

This allows looking the sendmail command at runtime in the PATH (eg: 'sendmail
-t') or using a custom command (eg: '/usr/local/bin/mysendmail -x').

* build-aux/guix.scm: Remove 'which' from native-inputs of the package.
* configure.ac: Add '--with-sendmail' option to set the command of sendmail
directly.
* NEWS: Document it.
* src/mcron/redirect.scm (with-mail-out): Don't add USER to the command of
sendmail.
---
 NEWS                   |  5 +++++
 build-aux/guix.scm     |  3 +--
 configure.ac           | 28 ++++++++--------------------
 src/mcron/redirect.scm |  7 +++----
 4 files changed, 17 insertions(+), 26 deletions(-)

diff --git a/NEWS b/NEWS
index d3f31cd..14631c7 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,11 @@ GNU Mcron NEWS                                    -*- outline 
-*-
 
 * Noteworthy changes in release ?.? (????-??-??) [?]
 
+** Improvements
+
+  The "--with-sendmail" configure variable has been added, which allows
+  looking the sendmail command at runtime in the PATH (eg: 'sendmail -t') or
+  using a custom command (eg: '/usr/local/bin/mysendmail -x').
 
 * Noteworthy changes in release 1.1.1 (2018-04-08) [stable]
 
diff --git a/build-aux/guix.scm b/build-aux/guix.scm
index d90e0be..53bf570 100644
--- a/build-aux/guix.scm
+++ b/build-aux/guix.scm
@@ -62,5 +62,4 @@
      ("automake" ,(specification->package "automake"))
      ("help2man" ,(specification->package "help2man"))
      ("pkg-config" ,(specification->package "pkg-config"))
-     ("texinfo" ,(specification->package "texinfo"))
-     ("which" ,(specification->package "which")))))
+     ("texinfo" ,(specification->package "texinfo")))))
diff --git a/configure.ac b/configure.ac
index 0bb9262..0645e25 100644
--- a/configure.ac
+++ b/configure.ac
@@ -55,26 +55,14 @@ GUILE_PROGS
 
 AM_MISSING_PROG(HELP2MAN, help2man, $missing_dir)
 
-# Now find a sendmail or equivalent.
-
-AC_CHECK_PROGS(SENDMAIL, sendmail)
-if test "x$ac_cv_prog_SENDMAIL" != "x"; then
-   AC_MSG_CHECKING(sendmail path and arguments)
-   ac_cv_prog_SENDMAIL="`$ac_cv_prog_WHICH sendmail` -FCronDaemon -odi -oem "
-dnl  -or0s"
-   AC_MSG_RESULT($ac_cv_prog_SENDMAIL)
-
-else
-   AC_CHECK_PROGS(SENDMAIL, mail)
-   if test "x$ac_cv_prog_SENDMAIL" != "x"; then
-      AC_MSG_CHECKING(mail path)
-      ac_cv_prog_SENDMAIL="`$ac_cv_prog_WHICH mail` -d "
-      AC_MSG_RESULT($ac_cv_prog_SENDMAIL)
-   else
-      AC_MSG_RESULT(No mail program found)
-   fi
-fi
-SENDMAIL=$ac_cv_prog_SENDMAIL
+AC_MSG_CHECKING([sendmail command])
+AC_ARG_WITH(sendmail,
+  [AS_HELP_STRING([--with-sendmail=COMMAND],
+    [command to read an email message from standard input, and send it])],
+  SENDMAIL=$withval,
+  SENDMAIL=["sendmail -t"])
+AC_MSG_RESULT($SENDMAIL)
+AC_SUBST(SENDMAIL)
 
 AC_ARG_ENABLE([multi-user],
   [AS_HELP_STRING([--disable-multi-user],
diff --git a/src/mcron/redirect.scm b/src/mcron/redirect.scm
index 6711407..2378fd3 100644
--- a/src/mcron/redirect.scm
+++ b/src/mcron/redirect.scm
@@ -170,10 +170,9 @@
           (set-current-output-port (if (and (string? mailto)
                                             (string=? mailto ""))
                                        (open-output-file "/dev/null")
-                                       (open-output-pipe
-                                          (string-append config-sendmail
-                                                         " "
-                                                         user))))
+                                       ;; The sendmail command should read
+                                       ;; recipients from the message header.
+                                       (open-output-pipe config-sendmail)))
           (set-current-input-port (car child->parent))
           (display "To: ") (display user) (newline)
           (display "From: mcron") (newline)
-- 
2.19.0