bug#39953: 27.0.50; [PATCH] explicitly fail if Emacs was configured with but cannot compile native JSON support

bug#39953: 27.0.50; [PATCH] explicitly fail if Emacs was configured with but cannot compile native JSON support

[Stéphane Maniaci]

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

Hello,

I was trying to give the Emacs 27 pre-test a spin, notably for the
native JSON support, and it took me a long time to figure out it
wasn't actually being configured for it as I was missing the Jansson
library headers.

This patch explicitly fails ./configure if `--with-json' is passed but
cannot be fulfilled. Which means users without the Jansson headers
won't be able to compile Emacs unless explicitly disabling it with
`--without-json'; I don't know if that is too extreme, maybe
AC_MSG_WARN would be more appropriate, but as long as it's notified in
some way to the user I'm happy.

It's also my very first patch so please excuse any mistakes in the
protocol/patch.

Cheers,
Stéphane.

[-- Attachment #2: 0001-configure.ac-warn-users-if-they-can-t-compile-native.patch --]
[-- Type: application/octet-stream, Size: 1187 bytes --]

From 5bc2c6d3a3a805a73577c869b1480757b0a85f56 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Maniaci?= <stephane.maniaci@gmail.com>
Date: Fri, 6 Mar 2020 11:30:44 +0000
Subject: [PATCH] configure.ac: warn users if they can't compile native JSON
 support

Running ``./configure --with-json'' is successful even when the
required Jansson headers are not present. The only information about
it is the line ``Does Emacs use -ljansson?'' at the end of the process
which might not cognate with the user.

Avoid any confusion by explicitly failing if native JSON support has
been requested but cannot be provided.

* configure.ac: warn users if they can't compile native JSON support.
---
 configure.ac | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/configure.ac b/configure.ac
index a4daf1414d..8ae0f70140 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2942,6 +2942,8 @@ if test "${with_json}" = yes; then
   if test "${HAVE_JSON}" = yes; then
     AC_DEFINE(HAVE_JSON, 1, [Define if using Jansson.])
     JSON_OBJ=json.o
+  else
+    AC_MSG_ERROR([Native JSON support requested but Jansson headers not found.])
   fi
 
   # Windows loads libjansson dynamically
-- 
2.23.0

bug#39953: 27.0.50; [PATCH] explicitly fail if Emacs was configured with but cannot compile native JSON support

[Eli Zaretskii]

> From: Stéphane Maniaci <stephane.maniaci@gmail.com>
> Date: Fri, 6 Mar 2020 12:13:17 +0000
> 
> This patch explicitly fails ./configure if `--with-json' is passed but
> cannot be fulfilled. Which means users without the Jansson headers
> won't be able to compile Emacs unless explicitly disabling it with
> `--without-json'; I don't know if that is too extreme, maybe
> AC_MSG_WARN would be more appropriate, but as long as it's notified in
> some way to the user I'm happy.

I think such handling of optional libraries is generally considered as
annoyance, so we only do that for some specific libraries.

Let's see what others think about this proposal.

bug#39953: 27.0.50; [PATCH] explicitly fail if Emacs was configured with but cannot compile native JSON support

[Noam Postavsky]

>> This patch explicitly fails ./configure if `--with-json' is passed but
>> cannot be fulfilled. Which means users without the Jansson headers
>> won't be able to compile Emacs unless explicitly disabling it with
>> `--without-json'; I don't know if that is too extreme,

This seems a bit too extreme to me, but I think giving an error on
missing Jansson headers only if --with-json is explicitly passed would
be okay.

bug#39953: 27.0.50; [PATCH] explicitly fail if Emacs was configured with but cannot compile native JSON support

[Noam Postavsky]

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

Noam Postavsky <npostavs@gmail.com> writes:

>>> This patch explicitly fails ./configure if `--with-json' is passed but
>>> cannot be fulfilled. Which means users without the Jansson headers
>>> won't be able to compile Emacs unless explicitly disabling it with
>>> `--without-json'; I don't know if that is too extreme,
>
> This seems a bit too extreme to me, but I think giving an error on
> missing Jansson headers only if --with-json is explicitly passed would
> be okay.

I was reminded about this from some reddit posts:

https://old.reddit.com/r/emacs/comments/fwgpkd/weekly_tipstricketc_thread/fmo9d5v/

Here's a proposed patch implementing my suggestion above.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 3112 bytes --]

From 7161e9ae2e55d2624ec60d33a467af6d9960d6bb Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 11 Apr 2020 09:33:20 -0400
Subject: [PATCH] Make --with-json imply than jansson support is mandatory
 (Bug#39953)

Currently, it is easy to miss that jansson support was not included,
since configure succeeds if the jansson development files (headers,
etc) are missing.  Failing when jansson is missing by default would be
a bit extreme, so this is a compromise; we only fail if --with-json
was explicitly passed.
* configure.ac (OPTION_DEFAULT_IFAVAILABLE): New macro.  Use it to
define the --with-json option.  Add with_json and HAVE_JSON to the
'MISSING' checks.
---
 configure.ac | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 73243001ba..f4b01e833b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -219,6 +219,21 @@ AC_DEFUN
     m4_bpatsubst([with_$1], [[^0-9a-z]], [_])=no])dnl
 ])dnl
 
+dnl OPTION_DEFAULT_IFAVAILABLE(NAME, HELP-STRING)
+dnl Create a new --with option that defaults to 'ifavailable'.
+dnl NAME is the base name of the option.  The shell variable with_NAME
+dnl   will be set to either the user's value (if the option is
+dnl   specified; 'yes' for a plain --with-NAME) or to 'ifavailable' (if the
+dnl   option is not specified).  Note that the shell variable name is
+dnl   constructed as autoconf does, by replacing non-alphanumeric
+dnl   characters with "_".
+dnl HELP-STRING is the help text for the option.
+AC_DEFUN([OPTION_DEFAULT_IFAVAILABLE], [dnl
+  AC_ARG_WITH([$1],[AS_HELP_STRING([--with-$1],[$2])],[],[dnl
+    m4_bpatsubst([with_$1], [[^0-9a-z]], [_])=ifavailable])dnl
+])dnl
+
+
 dnl OPTION_DEFAULT_ON(NAME, HELP-STRING)
 dnl Create a new --with option that defaults to $with_features.
 dnl NAME is the base name of the option.  The shell variable with_NAME
@@ -433,7 +448,7 @@ AC_DEFUN
 OPTION_DEFAULT_OFF([cairo],[compile with Cairo drawing])
 OPTION_DEFAULT_ON([xml2],[don't compile with XML parsing support])
 OPTION_DEFAULT_OFF([imagemagick],[compile with ImageMagick image support])
-OPTION_DEFAULT_ON([json], [don't compile with native JSON support])
+OPTION_DEFAULT_IFAVAILABLE([json], [don't compile with native JSON support])
 
 OPTION_DEFAULT_ON([xft],[don't use XFT for anti aliased fonts])
 OPTION_DEFAULT_ON([harfbuzz],[don't use HarfBuzz for text shaping])
@@ -2950,7 +2965,7 @@ AC_DEFUN
 HAVE_JSON=no
 JSON_OBJ=
 
-if test "${with_json}" = yes; then
+if test "${with_json}" != no; then
   EMACS_CHECK_MODULES([JSON], [jansson >= 2.7],
     [HAVE_JSON=yes], [HAVE_JSON=no])
   if test "${HAVE_JSON}" = yes; then
@@ -3893,6 +3908,11 @@ AC_DEFUN
   *) MISSING="$MISSING gnutls"
      WITH_IFAVAILABLE="$WITH_IFAVAILABLE --with-gnutls=ifavailable";;
 esac
+case $with_json,$HAVE_JSON in
+  no,* | ifavailable,* | *,yes) ;;
+  *) MISSING="$MISSING json"
+     WITH_IFAVAILABLE="$WITH_IFAVAILABLE --with-json=ifavailable";;
+esac
 if test "X${MISSING}" != X; then
   AC_MSG_ERROR([The following required libraries were not found:
     $MISSING
-- 
2.11.0

bug#39953: 27.0.50; [PATCH] explicitly fail if Emacs was configured with but cannot compile native JSON support

[Stefan Kangas]

Noam Postavsky <npostavs@gmail.com> writes:

>> This seems a bit too extreme to me, but I think giving an error on
>> missing Jansson headers only if --with-json is explicitly passed would
>> be okay.
>
> I was reminded about this from some reddit posts:
>
> https://old.reddit.com/r/emacs/comments/fwgpkd/weekly_tipstricketc_thread/fmo9d5v/
>
> Here's a proposed patch implementing my suggestion above.

My knowledge about the build system is rudimentary at best, so I can't
comment on your implementation.  The behaviour you suggest sounds
reasonable though.

Best regards,
Stefan Kangas

bug#39953: 27.0.50; [PATCH] explicitly fail if Emacs was configured with but cannot compile native JSON support

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> I was reminded about this from some reddit posts:
>
> https://old.reddit.com/r/emacs/comments/fwgpkd/weekly_tipstricketc_thread/fmo9d5v/
>
> Here's a proposed patch implementing my suggestion above.
>
>>From 7161e9ae2e55d2624ec60d33a467af6d9960d6bb Mon Sep 17 00:00:00 2001
> From: Noam Postavsky <npostavs@gmail.com>
> Date: Sat, 11 Apr 2020 09:33:20 -0400
> Subject: [PATCH] Make --with-json imply than jansson support is mandatory
>  (Bug#39953)

This sounds like a good idea to me, but this patch from April was never
applied?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39953: 27.0.50; [PATCH] explicitly fail if Emacs was configured with but cannot compile native JSON support

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>>>>From 7161e9ae2e55d2624ec60d33a467af6d9960d6bb Mon Sep 17 00:00:00 2001
>> From: Noam Postavsky <npostavs@gmail.com>
>> Date: Sat, 11 Apr 2020 09:33:20 -0400
>> Subject: [PATCH] Make --with-json imply than jansson support is mandatory
>>  (Bug#39953)
>
> This sounds like a good idea to me, but this patch from April was never
> applied?

I applied the patch and did some testing, and it works for me, so I've
pushed the change to Emacs 28.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no