bug#48228: json-serialize should signal error when dll is not found [MS Windows]

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Dmitry Gutov]

Tags: patch

As discussed on Emacs Help.

Another commenter suggested signaling a specific error. Please advise 
what to name it and where to put it.

diff --git a/src/json.c b/src/json.c
index 3f1d27ad7f..ece057ae41 100644
--- a/src/json.c
+++ b/src/json.c
@@ -596,8 +596,7 @@ DEFUN ("json-serialize", Fjson_serialize, 
Sjson_serialize, 1, MANY,
      }
    if (!json_initialized)
      {
-      message1 ("jansson library not found");
-      return Qnil;
+      Fsignal (Qerror, list1 (build_unibyte_string ("jansson library 
not found")));
      }
  #endif

@@ -707,8 +706,7 @@ DEFUN ("json-insert", Fjson_insert, Sjson_insert, 1, 
MANY,
      }
    if (!json_initialized)
      {
-      message1 ("jansson library not found");
-      return Qnil;
+      Fsignal (Qerror, list1 (build_unibyte_string ("jansson library 
not found")));
      }
  #endif

@@ -966,8 +964,7 @@ DEFUN ("json-parse-string", Fjson_parse_string, 
Sjson_parse_string, 1, MANY,
      }
    if (!json_initialized)
      {
-      message1 ("jansson library not found");
-      return Qnil;
+      Fsignal (Qerror, list1 (build_unibyte_string ("jansson library 
not found")));
      }
  #endif

@@ -1065,8 +1062,7 @@ DEFUN ("json-parse-buffer", Fjson_parse_buffer, 
Sjson_parse_buffer,
      }
    if (!json_initialized)
      {
-      message1 ("jansson library not found");
-      return Qnil;
+      Fsignal (Qerror, list1 (build_unibyte_string ("jansson library 
not found")));
      }
  #endif

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Philipp Stephani]

Am Di., 4. Mai 2021 um 17:33 Uhr schrieb Dmitry Gutov <dgutov@yandex.ru>:
> Another commenter suggested signaling a specific error. Please advise
> what to name it and where to put it.

Maybe json-unavailable? json.c already defines several other error symbols.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Dmitry Gutov]

On 04.05.2021 18:46, Philipp Stephani wrote:
> Maybe json-unavailable? json.c already defines several other error symbols.

It might make sense to define a common error that other features (like 
xml.c) would use in similar situation.

But I'm fine with a package-specific error, too.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 4 May 2021 18:49:14 +0300
> Cc: 48228@debbugs.gnu.org
> 
> On 04.05.2021 18:46, Philipp Stephani wrote:
> > Maybe json-unavailable? json.c already defines several other error symbols.
> 
> It might make sense to define a common error that other features (like 
> xml.c) would use in similar situation.

An alternative to signaling an error would be to provide a
json-avail-p functon which applications could test.  That would be
similar to what we do with other libraries, like image libraries and
GnuTLS.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Dmitry Gutov]

On 04.05.2021 19:08, Eli Zaretskii wrote:
> An alternative to signaling an error would be to provide a
> json-avail-p functon which applications could test.  That would be
> similar to what we do with other libraries, like image libraries and
> GnuTLS.

That's an option.

I'd rather we chose a less error-prone approach, though. No pun intended.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Robert Pluim]

>>>>> On Tue, 4 May 2021 19:10:27 +0300, Dmitry Gutov <dgutov@yandex.ru> said:

    Dmitry> On 04.05.2021 19:08, Eli Zaretskii wrote:
    >> An alternative to signaling an error would be to provide a
    >> json-avail-p functon which applications could test.  That would be
    >> similar to what we do with other libraries, like image libraries and
    >> GnuTLS.

    Dmitry> That's an option.

    Dmitry> I'd rather we chose a less error-prone approach, though. No pun intended.

What makes it error-prone? Those existing testing functions (on
Windows) attempt to load the relevant DLL's using the exact same
mechanisms as the actual code, so the failure (and success) modes are
identical.

Robert
-- 

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, 48228@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 4 May 2021 19:10:27 +0300
> 
> On 04.05.2021 19:08, Eli Zaretskii wrote:
> > An alternative to signaling an error would be to provide a
> > json-avail-p functon which applications could test.  That would be
> > similar to what we do with other libraries, like image libraries and
> > GnuTLS.
> 
> That's an option.
> 
> I'd rather we chose a less error-prone approach, though. No pun intended.

OTOH, we will next have people asking why some libraries use one
paradigm and others another...

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Dmitry Gutov]

On 04.05.2021 19:43, Robert Pluim wrote:
> What makes it error-prone? Those existing testing functions (on
> Windows) attempt to load the relevant DLL's using the exact same
> mechanisms as the actual code, so the failure (and success) modes are
> identical.

When somebody write code using json-serialize, and it can't do what it 
was asked to do, it should raise an error.

I have code like this in a separate project:

   (cond ((fboundp 'json-parse-buffer)
          (json-parse-buffer
           :array-type 'list
           :object-type 'alist
           :null-object nil))
         (t
          (let ((json-array-type 'list))
            (json-read))))

It has been there for a couple of years. And only now I find out that it 
can fail on MS Windows, because that failure is not reproducible on any 
other platform. I can only hope that whatever random Windows users 
encountered it saw the message wherever it was printed, and it was not 
concealed by subsequent messages from the caller code (reporting some 
cryptic errors, I imagine).

That's not to say we must not have a function json-available-p. If it's 
helpful, why not add it as well.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Dmitry Gutov]

On 04.05.2021 19:44, Eli Zaretskii wrote:
> OTOH, we will next have people asking why some libraries use one
> paradigm and others another...

Why not make them use both paradigms?

Have them raise error when a feature is not available *and* have 
xyz-available-p functions.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Eli Zaretskii]

> Cc: Eli Zaretskii <eliz@gnu.org>, p.stephani2@gmail.com, 48228@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 4 May 2021 19:59:15 +0300
> 
> On 04.05.2021 19:43, Robert Pluim wrote:
> > What makes it error-prone? Those existing testing functions (on
> > Windows) attempt to load the relevant DLL's using the exact same
> > mechanisms as the actual code, so the failure (and success) modes are
> > identical.
> 
> When somebody write code using json-serialize, and it can't do what it 
> was asked to do, it should raise an error.
> 
> I have code like this in a separate project:
> 
>    (cond ((fboundp 'json-parse-buffer)
>           (json-parse-buffer
>            :array-type 'list
>            :object-type 'alist
>            :null-object nil))
>          (t
>           (let ((json-array-type 'list))
>             (json-read))))
> 
> It has been there for a couple of years. And only now I find out that it 
> can fail on MS Windows, because that failure is not reproducible on any 
> other platform.

How is that different from similar code that relies on, say, librsvg
to display SVG images?

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, 48228@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 4 May 2021 20:00:37 +0300
> 
> On 04.05.2021 19:44, Eli Zaretskii wrote:
> > OTOH, we will next have people asking why some libraries use one
> > paradigm and others another...
> 
> Why not make them use both paradigms?

Could be okay, but it is no longer a local change in json.c, it will
be something affecting a lot of use cases out there.

For example, trying to display an image when its library isn't
available currently shows a placebo, but it will signal an error under
your suggestion.  If people aren't worried about that, we could try
it.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Dmitry Gutov]

On 04.05.2021 20:42, Eli Zaretskii wrote:
> How is that different from similar code that relies on, say, librsvg
> to display SVG images?

Does it have a Lisp entry point? If so, I suppose it should be fixed too.

My main experience with librsvg is creating image specs manually and 
having them used via the 'display' text property. There is no obvious 
place to signal an error in that scenario.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Eli Zaretskii]

> Cc: rpluim@gmail.com, p.stephani2@gmail.com, 48228@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 4 May 2021 20:47:26 +0300
> 
> On 04.05.2021 20:42, Eli Zaretskii wrote:
> > How is that different from similar code that relies on, say, librsvg
> > to display SVG images?
> 
> Does it have a Lisp entry point? If so, I suppose it should be fixed too.

We have create-image, which currently explicitly checks for the
relevant library to be available to Emacs.

> My main experience with librsvg is creating image specs manually and 
> having them used via the 'display' text property. There is no obvious 
> place to signal an error in that scenario.

There is: in create-image.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Dmitry Gutov]

On 04.05.2021 21:07, Eli Zaretskii wrote:
>> Cc: rpluim@gmail.com, p.stephani2@gmail.com, 48228@debbugs.gnu.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Tue, 4 May 2021 20:47:26 +0300
>>
>> On 04.05.2021 20:42, Eli Zaretskii wrote:
>>> How is that different from similar code that relies on, say, librsvg
>>> to display SVG images?
>>
>> Does it have a Lisp entry point? If so, I suppose it should be fixed too.
> 
> We have create-image, which currently explicitly checks for the
> relevant library to be available to Emacs.
> 
>> My main experience with librsvg is creating image specs manually and
>> having them used via the 'display' text property. There is no obvious
>> place to signal an error in that scenario.
> 
> There is: in create-image.

It does make sense to signal an error in that case, too (with a 
dedicated error symbol).

A bit less critical than the JSON case, because the latter can 
erroneously return nil (and print a message) in situations where nil is 
a valid return value. And one can create an image spec by hand without 
calling create-image, so the "real" error is going to happen somewhere 
else anyway (during redisplay, I imagine).

So I would probably split this change into 2 commits: the essential 
places where no valid code should proceed when there is no support, and 
cases like create-image, to be easily reverted if we see significant 
complaints.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Nikolay Kudryavtsev]

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

I'd definitely enjoy a more standardized API for testing optional 
feature presence.

We already have system-configuration-features, seems like it's 
reasonable we get system-configuration-working-features(can't think of a 
good name), which would do X-available-p for every X in 
system-configuration-features.

But first we'd have to get X-available-p(or some other convention) 
working for everything. Would it be acceptable for Emacs to ship with a 
small image in every supported format? Then imageX-available-p can be 
implemented by opening that image and catching the failure in 
create-image. I know that the spash.* image is already shipped in most 
formats, even in bmp for some reason.

This would let you quickly test which features are working... Now I have 
to go through a checklist 
<https://github.com/sg2002/ms-windows-builder.el#emacs-optional-feature-checklist>. 
Then I'd be able to automate such a check, which would be pretty helpful.


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

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Eli Zaretskii]

> From: Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com>
> Cc: rpluim@gmail.com, p.stephani2@gmail.com, 48228@debbugs.gnu.org
> Date: Thu, 6 May 2021 18:26:49 +0300
> 
> But first we'd have to get X-available-p(or some other convention) working for everything.

We have it already for almost every X.

> Would it be
> acceptable for Emacs to ship with a small image in every supported format? Then imageX-available-p can
> be implemented by opening that image and catching the failure in create-image.

That shouldn't be necessary, since we already have image-type-available-p.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Nikolay Kudryavtsev]

Oh, cool.

I think only GMP is left without a clear way to test for then.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Eli Zaretskii]

> From: Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com>
> Cc: dgutov@yandex.ru, rpluim@gmail.com, p.stephani2@gmail.com,
>  48228@debbugs.gnu.org
> Date: Thu, 6 May 2021 19:13:32 +0300
> 
> Oh, cool.
> 
> I think only GMP is left without a clear way to test for then.

The GMP test isn't necessary, because Emacs offers the same
functionality even without it, using a Gnulib module.

The only library I know of that doesn't have a test is libjansson.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Nikolay Kudryavtsev]

On the other hand, it is quite possible that X years down the line the 
bundled miniGMP would get out of step with the main GMP and you'd have a 
reason to use one instead of the other. And having a run time check 
would help. Unlikely, but happens with libraries all the time.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Eli Zaretskii]

> From: Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com>
> Cc: dgutov@yandex.ru, rpluim@gmail.com, p.stephani2@gmail.com,
>  48228@debbugs.gnu.org
> Date: Thu, 6 May 2021 19:29:20 +0300
> 
> On the other hand, it is quite possible that X years down the line the 
> bundled miniGMP would get out of step with the main GMP and you'd have a 
> reason to use one instead of the other. And having a run time check 
> would help.

If that happens, we will indeed need to think about a predicate to
test that.  But for now it would be superfluous.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Nikolay Kudryavtsev]

IMHO it's better to have one now, so that if that case ever happens, 
package developers relying on GMP can work around it by using the 
predicate without waiting for a new Emacs version with a predicate to 
ship. Especially considering how slow the user base is at upgrading.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Eli Zaretskii]

> From: Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com>
> Cc: dgutov@yandex.ru, rpluim@gmail.com, p.stephani2@gmail.com,
>  48228@debbugs.gnu.org
> Date: Thu, 6 May 2021 19:42:28 +0300
> 
> IMHO it's better to have one now, so that if that case ever happens, 
> package developers relying on GMP can work around it by using the 
> predicate without waiting for a new Emacs version with a predicate to 
> ship. Especially considering how slow the user base is at upgrading.

You assume that this divergence will certainly happen, whereas I
assume it almost certainly won't, because the Gnulib module is
explicitly intended to be a replacement for GMP, and Emacs is its very
respected client.

It is a bad mantra to have APIs that return trivially known values,
there are people here that will sooner or later suggest to remove it
as redundant.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Nikolay Kudryavtsev]

You're probably right about the GMP in particular, but still I don't 
think that many people would go "Please remove gmp-available-p since I 
never use it". I'm just thinking that it's a good idea to have a unified 
API for tracking dynamic library features, considering most of it 
already exists in system-configuration-features. It does not even have 
to be a top level function for every feature, maybe something like 
(feature-availble-p 'gmp) would do.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Dmitry Gutov]

On 06.05.2021 19:42, Nikolay Kudryavtsev wrote:
> IMHO it's better to have one now, so that if that case ever happens, 
> package developers relying on GMP can work around it by using the 
> predicate without waiting for a new Emacs version with a predicate to ship.

If that ever happens, the workaround will only be needed in the new 
version of Emacs (right?), so the same version could introduce the 
predicate, and whoever needs it would just test it with fboundp first.

Which they'd need to do anyway, if they support previous versions of Emacs.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Nikolay Kudryavtsev]

06.05.2021 20:11, Dmitry Gutov wrote:
>
> If that ever happens, the workaround will only be needed in the new 
> version of Emacs (right?), so the same version could introduce the 
> predicate, and whoever needs it would just test it with fboundp first. 

You should not assume that the predicate would be introduced by the same 
version in which the incompatibility first happens. Let's say the new 
GMP got released 5 minutes ago, and my package that relies on it is 
already broken and I have to code the workaround, but I can't properly 
dispatch it since 27.2 does not have that gmp-available-p, so I have to 
write my own explicit test.

Eli is probably correct in that this would never happen in practice for 
GMP, but what I'm saying is that it's a good idea to have a single 
unified convention for testing every single dynamic library feature, so 
that if someone codes library X support we could expect it to follow the 
same convention and this would give us some protection from such 
problems in addition to giving you a quick way to see if whatever 
distro's Emacs binary you're currently using is properly configured.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Eli Zaretskii]

> From: Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com>
> Cc: rpluim@gmail.com, p.stephani2@gmail.com, 48228@debbugs.gnu.org
> Date: Thu, 6 May 2021 20:36:10 +0300
> 
> Eli is probably correct in that this would never happen in practice for 
> GMP, but what I'm saying is that it's a good idea to have a single 
> unified convention for testing every single dynamic library feature

GMP is different in this aspect from other libraries: Emacs built with
GMP will refuse to start if the GMP DLL is not available.

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > On the other hand, it is quite possible that X years down the line the 
  > bundled miniGMP would get out of step with the main GMP and you'd have a 
  > reason to use one instead of the other. And having a run time check 
  > would help.

I wonder if we can help the maintainers remember to keep the two
libraries in sync.  Perhaps the test suite for GMP could include tests
that run miniGMP and compare the results.

Those tests would have a chance of detecting a discrepancy.  But, more
than that, they could direct the attention of developers of GMP towards
the need to keep miniGMP in sync with it.

This just occurred to me, and I don't know whether it has ever been
tried.  But I think it can't hurt much.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> As discussed on Emacs Help.
>
> Another commenter suggested signaling a specific error. Please advise
> what to name it and where to put it.
>
> diff --git a/src/json.c b/src/json.c
> index 3f1d27ad7f..ece057ae41 100644
> --- a/src/json.c
> +++ b/src/json.c
> @@ -596,8 +596,7 @@ DEFUN ("json-serialize", Fjson_serialize,
> Sjson_serialize, 1, MANY,
>      }
>    if (!json_initialized)
>      {
> -      message1 ("jansson library not found");
> -      return Qnil;
> +      Fsignal (Qerror, list1 (build_unibyte_string ("jansson library
> not found")));

I've now applied a version of this change to Emacs 28 (with a new error
symbol).

The discussion here then turned towards the question of whether there
should be a `json-available-p' function, and there should.  But json.c
is slightly unusual in this respect that it's not compiled at all if
jansson isn't available, so the function will have to go somewhere else,
which is rather, er, inconvenient.

Anybody have an idea how to solve that problem?

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

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Anybody have an idea how to solve that problem?

Actually, I'll open a new bug report for that, since it's slightly
tangential.

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

bug#48228: json-serialize should signal error when dll is not found [MS Windows]

[Dmitry Gutov]

On 20.07.2021 15:50, Lars Ingebrigtsen wrote:
> I've now applied a version of this change to Emacs 28 (with a new error
> symbol).

Thanks, Lars!