Re: 01/01: guix: node-build-system: Use guile-json instead of a custom parser.

Re: 01/01: guix: node-build-system: Use guile-json instead of a custom parser.

[Ludovic Courtès]

Hi Julien,

guix-commits@gnu.org skribis:

> commit 8eb0ba532ebbebef23180e666e0607ea735f9c1a
> Author: Julien Lepiller <julien@lepiller.eu>
> Date:   Sun Jul 14 14:50:21 2019 +0200
>
>     guix: node-build-system: Use guile-json instead of a custom parser.
>     
>     * guix/build/json.scm: Remove file.
>     * Makefile.am: Remove it.
>     * guix/build/node-build-system.scm: Use (json parser) instead of (guix build json).
>     * guix/build-system/node.scm: Idem.

This commit log doesn’t spell out all the variables and entities that
were modified.  Please consider doing it next time, for clarity.

> +++ b/guix/build-system/node.scm
> @@ -18,7 +18,6 @@
>  
>  (define-module (guix build-system node)
>    #:use-module (guix store)
> -  #:use-module (guix build json)
>    #:use-module (guix build union)
>    #:use-module (guix utils)
>    #:use-module (guix packages)
> @@ -27,6 +26,7 @@
>    #:use-module (guix build-system)
>    #:use-module (guix build-system gnu)
>    #:use-module (ice-9 match)
> +  #:use-module (json parser)
>    #:export (npm-meta-uri
>              %node-build-system-modules
>              node-build
> @@ -40,8 +40,8 @@ registry."
>  (define %node-build-system-modules
>    ;; Build-side modules imported by default.
>    `((guix build node-build-system)
> -    (guix build json)
>      (guix build union)
> +    (json parser)

The effect of this change is to import the (json parser) from the host
side into the build side.

As a result, if I have installed Guile-JSON 1.2 and you have Guile-JSON
3.1, we’ll end up building different derivations (and one of them won’t
build :-)).

The solution here would be to do the equivalent of ‘with-extensions’ for
gexps.

However, given that that’s annoying to do without gexps, and given that
the plan is to move build systems to gexps Real Soon, I’d be in favor of
simply reverting this commit and using the custom JSON parser.  We can
add a TODO/FIXME explaining that we’ll replace it with Guile-JSON as
soon as build systems are rewritten to use gexps.

How does that sound?

Apologies if I overlooked it in your initial patch submission!

Thank you,
Ludo’.

Re: 01/01: guix: node-build-system: Use guile-json instead of a custom parser.

[Julien Lepiller]

Le Sun, 14 Jul 2019 15:40:43 +0200,
Ludovic Courtès <ludo@gnu.org> a écrit :

> Hi Julien,
> 
> guix-commits@gnu.org skribis:
> 
> > commit 8eb0ba532ebbebef23180e666e0607ea735f9c1a
> > Author: Julien Lepiller <julien@lepiller.eu>
> > Date:   Sun Jul 14 14:50:21 2019 +0200
> >
> >     guix: node-build-system: Use guile-json instead of a custom
> > parser. 
> >     * guix/build/json.scm: Remove file.
> >     * Makefile.am: Remove it.
> >     * guix/build/node-build-system.scm: Use (json parser) instead
> > of (guix build json).
> >     * guix/build-system/node.scm: Idem.  
> 
> This commit log doesn’t spell out all the variables and entities that
> were modified.  Please consider doing it next time, for clarity.
> 
> > +++ b/guix/build-system/node.scm
> > @@ -18,7 +18,6 @@
> >  
> >  (define-module (guix build-system node)
> >    #:use-module (guix store)
> > -  #:use-module (guix build json)
> >    #:use-module (guix build union)
> >    #:use-module (guix utils)
> >    #:use-module (guix packages)
> > @@ -27,6 +26,7 @@
> >    #:use-module (guix build-system)
> >    #:use-module (guix build-system gnu)
> >    #:use-module (ice-9 match)
> > +  #:use-module (json parser)
> >    #:export (npm-meta-uri
> >              %node-build-system-modules
> >              node-build
> > @@ -40,8 +40,8 @@ registry."
> >  (define %node-build-system-modules
> >    ;; Build-side modules imported by default.
> >    `((guix build node-build-system)
> > -    (guix build json)
> >      (guix build union)
> > +    (json parser)  
> 
> The effect of this change is to import the (json parser) from the host
> side into the build side.
> 
> As a result, if I have installed Guile-JSON 1.2 and you have
> Guile-JSON 3.1, we’ll end up building different derivations (and one
> of them won’t build :-)).
> 
> The solution here would be to do the equivalent of ‘with-extensions’
> for gexps.
> 
> However, given that that’s annoying to do without gexps, and given
> that the plan is to move build systems to gexps Real Soon, I’d be in
> favor of simply reverting this commit and using the custom JSON
> parser.  We can add a TODO/FIXME explaining that we’ll replace it
> with Guile-JSON as soon as build systems are rewritten to use gexps.
> 
> How does that sound?
> 
> Apologies if I overlooked it in your initial patch submission!
> 
> Thank you,
> Ludo’.

OK, sorry for pushing this patch then... I've reverted it in
a4bb18921099b2ec8c1699e08a73ca0fa78d0486. Thanks for the explanation!

Re: 01/01: guix: node-build-system: Use guile-json instead of a custom parser.

[Robert Vollmert]

On 14. Jul 2019, at 20:19, Julien Lepiller <julien@lepiller.eu> wrote:
> 
> Le Sun, 14 Jul 2019 15:40:43 +0200,
> Ludovic Courtès <ludo@gnu.org> a écrit :
>> The effect of this change is to import the (json parser) from the host
>> side into the build side.
>> 
>> As a result, if I have installed Guile-JSON 1.2 and you have
>> Guile-JSON 3.1, we’ll end up building different derivations (and one
>> of them won’t build :-)).
>> 
>> The solution here would be to do the equivalent of ‘with-extensions’
>> for gexps.
>> 
>> However, given that that’s annoying to do without gexps, and given
>> that the plan is to move build systems to gexps Real Soon, I’d be in
>> favor of simply reverting this commit and using the custom JSON
>> parser.  We can add a TODO/FIXME explaining that we’ll replace it
>> with Guile-JSON as soon as build systems are rewritten to use gexps.
>> 
>> How does that sound?
>> 
>> Apologies if I overlooked it in your initial patch submission!
>> 
>> Thank you,
>> Ludo’.
> 
> OK, sorry for pushing this patch then... I've reverted it in
> a4bb18921099b2ec8c1699e08a73ca0fa78d0486. Thanks for the explanation!

It’s a bit my fault, for remarking on the fact that the cargo build
system does use guile-json. I suppose that’s not worth fixing either
at this point?

Cheers
Robert

Re: 01/01: guix: node-build-system: Use guile-json instead of a custom parser.

[Ludovic Courtès]

Hi,

Robert Vollmert <rob@vllmrt.net> skribis:

> It’s a bit my fault, for remarking on the fact that the cargo build
> system does use guile-json. I suppose that’s not worth fixing either
> at this point?

Indeed, ‘cargo-build-system’ has the same problem:

--8<---------------cut here---------------start------------->8---
(define %cargo-build-system-modules
  ;; Build-side modules imported by default.
  `((guix build cargo-build-system)
    (json parser)
    ,@%cargo-utils-modules))
--8<---------------cut here---------------end--------------->8---

We should definitely fix it, or we’ll run into discrepancies like I
described.

For now, the easiest option would be to use (guix build json) as well.

Could you look into it, Robert?

Thanks,
Ludo’.

Re: 01/01: guix: node-build-system: Use guile-json instead of a custom parser.

[Robert Vollmert]

> On 15. Jul 2019, at 14:25, Ludovic Courtès <ludo@gnu.org> wrote:
> 
> Hi,
> 
> Robert Vollmert <rob@vllmrt.net> skribis:
> 
>> It’s a bit my fault, for remarking on the fact that the cargo build
>> system does use guile-json. I suppose that’s not worth fixing either
>> at this point?
> 
> Indeed, ‘cargo-build-system’ has the same problem:
> 
> --8<---------------cut here---------------start------------->8---
> (define %cargo-build-system-modules
>  ;; Build-side modules imported by default.
>  `((guix build cargo-build-system)
>    (json parser)
>    ,@%cargo-utils-modules))
> --8<---------------cut here---------------end--------------->8---
> 
> We should definitely fix it, or we’ll run into discrepancies like I
> described.
> 
> For now, the easiest option would be to use (guix build json) as well.
> 
> Could you look into it, Robert?

Done: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=36676

(Was the cargo-build-system not even working a known issue? I don’t
see a bug report…)