[bug#51838,v6,05/41] guix: node-build-system: Add 'delete-dependencies' helper function.

* guix/build/node-build-system.scm (delete-dependencies): New exported
procedure.  Functionally updates a "package.json"-like value by removing
specified npm packages from the "dependencies" and "devDependencies"
objects.
---
 guix/build/node-build-system.scm | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Am Donnerstag, dem 30.12.2021 um 02:38 -0500 schrieb Philip McGrath:
> * guix/build/node-build-system.scm (delete-dependencies): New
> exported procedure.  Functionally updates a "package.json"-like value
> by removing specified npm packages from the "dependencies" and
> "devDependencies" objects.
> ---
>  guix/build/node-build-system.scm | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/guix/build/node-build-system.scm b/guix/build/node-
> build-system.scm
> index dc8b6a41c2..9967223b86 100644
> --- a/guix/build/node-build-system.scm
> +++ b/guix/build/node-build-system.scm
> @@ -27,6 +27,7 @@ (define-module (guix build node-build-system)
>    #:use-module (srfi srfi-1)
>    #:export (%standard-phases
>              with-atomic-json-file-replacement
> +            delete-dependencies
>              node-build))
>  
>  ;; Commentary:
> @@ -325,6 +326,23 @@ (define resolve-dependencies
>                                 deps))))))
>    #t)
>  
> +(define (delete-dependencies pkg-meta absent-dependencies)
> +  "Functionally update PKG-META, a json object corresponding to a
> +'package.json' file, to allow building without the ABSENT-
> DEPENDENCIES.  To
> +avoid reintroducing the ABSENT-DEPENDENCIES, only use this procedure
> after the
> +'patch-dependencies' phase."
> +  (define delete-fom-jsobject
> +    (match-lambda
> +      (('@ . alist)
> +       (cons '@ (filter (match-lambda
> +                          ((k . v)
> +                           (not (member k absent-dependencies))))
> +                        alist)))))
> +  (jsobject-update*
> +   pkg-meta
> +   "devDependencies" '(@) delete-fom-jsobject
> +   "dependencies" '(@) delete-fom-jsobject))
Given this rather easy definition in terms of our helper functions, I
think this procedure can do more.  Particularly, I'd argue that we can
define it as such:

(define* (delete-dependencies dependencies #:key (file "package.json")
                              (json-keys 
                               '("dependencies" "devDependencies"))
  "Remove DEPENDENCIES from JSON_KEYS in FILE."
  (with-atomic-json-file-replacement ...))

This would in turn make it easier to delete dependencies from #:phases,
eliminating the need to shorten it to #:absent-dependencies.  WDYT?

Hi,

On 12/30/21 12:29, Liliana Marie Prikler wrote:
> Am Donnerstag, dem 30.12.2021 um 02:38 -0500 schrieb Philip McGrath:
>> +(define (delete-dependencies pkg-meta absent-dependencies)
>> +  "Functionally update PKG-META, a json object corresponding to a
>> +'package.json' file, to allow building without the ABSENT-
>> DEPENDENCIES.  To
>> +avoid reintroducing the ABSENT-DEPENDENCIES, only use this procedure
>> after the
>> +'patch-dependencies' phase."
>> +  (define delete-fom-jsobject
>> +    (match-lambda
>> +      (('@ . alist)
>> +       (cons '@ (filter (match-lambda
>> +                          ((k . v)
>> +                           (not (member k absent-dependencies))))
>> +                        alist)))))
>> +  (jsobject-update*
>> +   pkg-meta
>> +   "devDependencies" '(@) delete-fom-jsobject
>> +   "dependencies" '(@) delete-fom-jsobject))
> Given this rather easy definition in terms of our helper functions, I
> think this procedure can do more.  Particularly, I'd argue that we can
> define it as such:
> 
> (define* (delete-dependencies dependencies #:key (file "package.json")
>                                (json-keys
>                                 '("dependencies" "devDependencies"))
>    "Remove DEPENDENCIES from JSON_KEYS in FILE."
>    (with-atomic-json-file-replacement ...))
> 
> This would in turn make it easier to delete dependencies from #:phases,
> eliminating the need to shorten it to #:absent-dependencies.  WDYT?
> 

I don't think '#:json-keys' would be helpful.

In my view, the high-level purpose of 'delete-dependencies', 
'#:absent-dependencies', or whatever is to gather our collective 
procedural knowledge about how to modify a "package.json" file to build 
a package without some of the dependencies its developers have declared 
and to encode that knowledge in a single, abstracted point of control in 
'node-build-system', so that authors of Node.js package definitions can 
simply specify which declared dependencies are absent and leave it to 
'node-build-system' to act accordingly. (I don't think it matters _why_ 
the dependencies are absent, i.e. whether we don't want the them or 
merely don't have them.)

In our experience so far, the necessary modification does concretely 
amount to "Remove DEPENDENCIES from JSON_KEYS in FILE.", but that is not 
the ultimate purpose of this code, and I think that description, along 
with '#:json-keys', ends up being simultaneously too flexible and too 
restrictive. It is unnecessarily flexible because, if a package author 
ever passes some other value for '#:json-keys', that would seem to 
indicate that there's some procedural knowledge missing from 
'node-build-system', and it should be added there. More significantly, 
it unnecessarily seems to restrict 'delete-dependencies' from taking 
other kinds of actions to handle the absent dependencies, if in the 
future we should discover that there's something we need to do that 
wouldn't amount to just adding another JSON key. It's a little odd to 
give an example of something we might not know, but, for example, I 
could imagine learning that correct handling of absent 
"peerDependencies" could require more involved transformation of the 
structures under "peerDependenciesMeta".

As far as the rest of your suggestion, on the one hand, this:

   (define* (delete-dependencies deps #:key (file "package.json"))
     (with-atomic-json-file-replacement ...))

seems like a fine enhancement, and I could live with it---I'd even 
prefer it, if v6 but not v7 of this patch series can achieve consensus.

On the other hand, at the risk of beating a dead horse, it seems like a 
tiny step from the above to:

   (define* ((delete-dependencies deps #:key (file "package.json")) . _)
     (with-atomic-json-file-replacement ...))

which is just another name for 'make-delete-dependencies-phase', which 
AIUI you had found objectionable. (Apparently that shorthand would need 
(ice-9 curried-definitions).)

Indeed, if we observe that '#:file', similarly to '#:json-keys', will 
never be anything _other_ than "package.json", we could further simplify to:

   (define* ((delete-dependencies deps) . _)
     (with-atomic-json-file-replacement ...))

at which point we've basically re-invented the implementation of patch 
v7 05/41, which basically amounts to:

   (define* (delete-dependencies #:key absent-dependencies)
     (with-atomic-json-file-replacement ...))

In other words, I don't agree that any of these possible changes would 
"eliminat[e] the need to shorten it to #:absent-dependencies",

I still feel that there's something I'm fundamentally not understanding 
about your objections to '#:make-absent-dependencies', which is why, in 
v6, I tried to do exactly as you had proposed:

On 12/20/21 17:00, Liliana Marie Prikler wrote:
 > Hi Timothy,
 >
 > Am Montag, dem 20.12.2021 um 15:15 -0500 schrieb Timothy Sample:
 >> Hi Philip,
 >>
 >> Philip McGrath <philip@philipmcgrath.com> writes:
 >>
 >>> If we took your final suggestion above, I think we'd have something
 >>> like this:
 >>>
 >>> ```
 >>> #:phases
 >>> (modify-phases %standard-phases
 >>>    (add-after 'unpack 'delete-dependencies
 >>>      (make-delete-dependencies-phase '("node-tap"))))
 >>> ```
 >>
 >> I’m perfectly happy with this if it’s a compromise we all can agree on.
 >> It is exactly what popped into my imagination when I read Liliana’s
 >> suggestion.  I guess the one thing missing is that it would not
 >> necessarily be implemented on top of better “package.json”
 >> manipulation support.  That said, it doesn’t preclude providing that
 >> support if/when the need arises.
 > In my personal opinion, we would write that support first and perhaps
 > the shorthands later.  I.e.
 >
 > (add-after 'patch-dependencies 'drop-junk
 >    (lambda _
 >      (with-atomic-json-replacement "package.json"
 >        (lambda (json) (delete-dependencies json '("node-tap"))))))

Certainly I do agree that it would be better to support code more 
concise than that! But I think all of these variations are strictly 
worse than '#:absent-dependencies'. It isn't just that they are more 
typing: the require authors of package definitions to have some (not 
very much, but some) procedural knowledge about _how_ 
'node-build-system' deals with the absence of dependencies, rather than 
with '#:absent-dependencies', declaratively specifying _what_ is to be 
done. For example, as I mentioned in my cover letter at 
<https://issues.guix.gnu.org/51838#257>, even my own code from the 
exchange I just quoted:

 >>>    (add-after 'unpack 'delete-dependencies
 >>>      (make-delete-dependencies-phase '("node-tap"))))

would be broken in v6, because the implementation of 
'delete-dependencies' assumes that the 'patch-dependencies' phase has 
already been run. I think this is an implementation detail that users of 
'node-build-system' should not be required to know! Indeed, I think that 
would be a good reason to have 'patch-dependencies' handle the absent 
dependencies itself, as previous versions of this series have done, but 
at least making 'delete-dependencies' a phase in 'standard-phases', as 
v7 does, relieves the user of burden of managing the ordering 
requirement manually.

I expect the majority of Guix's Node.js packages will continue for the 
foreseeable future to need the functionality for absent dependencies I 
described at the beginning of this email, so I think we should provide 
it through a mechanism that is as high-level, concise, declarative as 
possible, and ideally one that will facilitate automated code generation 
and static analysis. On each of these criteria, I think 
'#:absent-dependencies' is better than any of the other proposals I've 
heard.

But, as I said, in the interest of compromise and moving forward, I'm 
willing to live with something based on v6 for now if that's what can 
achieve consensus, and then propose '#:absent-dependencies' separately. 
So, if you want me to send a new version with one of these other 
variations, tell me which one, and I'll do it.

I hope my tone isn't coming across the wrong way---I really don't mean 
to be snarky! But I am genuinely struggling to understand the 
significance of the difference between:

 >>>    (add-after 'unpack 'delete-dependencies
 >>>      (make-delete-dependencies-phase '("node-tap"))))

which I thought you objected to, and the result of what I think you've 
most recently proposed:

     (add-after 'patch-dependencies 'delete-dependencies
       (lambda () (delete-dependencies '("node-tap"))))

which would have avoided my earlier reservations about making the JSON 
representation part of the build system's public API for the first time.

So I'm not feeling very confident in my ability to predict what changes 
would or would not block consensus.

-Philip

Hi,

Am Donnerstag, dem 30.12.2021 um 20:09 -0500 schrieb Philip McGrath:
> Hi,
> 
> On 12/30/21 12:29, Liliana Marie Prikler wrote:
> > Am Donnerstag, dem 30.12.2021 um 02:38 -0500 schrieb Philip
> > McGrath:
> > > +  (define delete-fom-jsobject
For the record, I forgot to mention the typo here.  It obviously ought
to be delete-from-jsobject.

> [...]
> I don't think '#:json-keys' would be helpful.
> 
> In my view, the high-level purpose of 'delete-dependencies', 
> '#:absent-dependencies', or whatever is to gather our collective 
> procedural knowledge about how to modify a "package.json" file to
> build a package without some of the dependencies its developers have
> declared and to encode that knowledge in a single, abstracted point
> of control in 'node-build-system', so that authors of Node.js package
> definitions can simply specify which declared dependencies are absent
> and leave it to 'node-build-system' to act accordingly. (I don't
> think it matters _why_ the dependencies are absent, i.e. whether we
> don't want the them or merely don't have them.)
For the record, you can delete present dependencies as well, which is
one shortcoming in the "absent dependencies" metaphor.  As for deleting
absent packages automatically without specifying them, one could argue
that a shortcoming of this set is that it doesn't provide a way of
doing that.  I'll take a closer look at the ease-of-use as I walk down
your reply.

> In our experience so far, the necessary modification does concretely 
> amount to "Remove DEPENDENCIES from JSON_KEYS in FILE.", but that is
> not the ultimate purpose of this code, and I think that description,
> along with '#:json-keys', ends up being simultaneously too flexible
> and too restrictive. 
Admittedly, if you read DEPENDENCIES, JSON_KEYS and FILE as mere
variables with no meaning that's bad, but the key here is that
DEPENDENCIES is a list of dependencies and JSON_KEYS is a list of json
keys that refer to dependency lists.  Anything else would be unlawful
use.

> It is unnecessarily flexible because, if a package author ever passes
> some other value for '#:json-keys', that would seem to indicate that
> there's some procedural knowledge missing from 'node-build-system',
> and it should be added there.
The reason I have it as such is that a packager might decide in the
future to e.g. only drop a peer dependency, that might share a name
with a dev dependency or something along those lines.  Along with
#:file, it's also supposed to guard against files other than
package.json containing a list of dependencies that blows up in our
faces, which we could edit with the same primitive.  That being said, I
do get your point in that it's overly flexible for now in which the
point is just to rewrite package.json.

> More significantly, it unnecessarily seems to restrict 'delete-
> dependencies' from taking other kinds of actions to handle the absent
> dependencies, if in the future we should discover that there's
> something we need to do that wouldn't amount to just adding another
> JSON key. It's a little odd to give an example of something we might
> not know, but, for example, I could imagine learning that correct
> handling of absent "peerDependencies" could require more involved
> transformation of the structures under "peerDependenciesMeta".
Do you mean that as in "if a dependency is found under this key, then
foo, bar and baz also need to happen"?  If so, that would be
concerning, but also a good reason to have that abstraction.

> As far as the rest of your suggestion, on the one hand, this:
> 
>    (define* (delete-dependencies deps #:key (file "package.json"))
>      (with-atomic-json-file-replacement ...))
> 
> seems like a fine enhancement, and I could live with it---I'd even 
> prefer it, if v6 but not v7 of this patch series can achieve
> consensus.
Given that you deleted keys, I don't think there's a good reason to
keep around file... or can it be located in a subdirectory?

> On the other hand, at the risk of beating a dead horse, it seems like
> a tiny step from the above to:
> 
>    (define* ((delete-dependencies deps #:key (file "package.json")) .
> _)
>      (with-atomic-json-file-replacement ...))
> 
> which is just another name for 'make-delete-dependencies-phase',
> which AIUI you had found objectionable. (Apparently that shorthand
> would need (ice-9 curried-definitions).)
The reason to not use a phase writer here, is that you could combine it
with other stuff in a single phase.  E.g.

  (add-after 'unpack 'remove-foo
    (lambda _
      (delete-dependencies '("foo" "bar" "baz"))
      (delete-file "baz-loader.js")))

> Indeed, if we observe that '#:file', similarly to '#:json-keys', will
> never be anything _other_ than "package.json", we could further
> simplify to:
> 
>    (define* ((delete-dependencies deps) . _)
>      (with-atomic-json-file-replacement ...))
> 
> at which point we've basically re-invented the implementation of
> patch v7 05/41, which basically amounts to:
> 
>    (define* (delete-dependencies #:key absent-dependencies)
>      (with-atomic-json-file-replacement ...))
> 
> In other words, I don't agree that any of these possible changes
> would "eliminat[e] the need to shorten it to #:absent-dependencies",
Sorry for the typo.

> I still feel that there's something I'm fundamentally not
> understanding about your objections to '#:make-absent-dependencies',
> which is why, in v6, I tried to do exactly as you had proposed:
> 
> On 12/20/21 17:00, Liliana Marie Prikler wrote:
>  > Hi Timothy,
>  >
>  > Am Montag, dem 20.12.2021 um 15:15 -0500 schrieb Timothy Sample:
>  >> Hi Philip,
>  >>
>  >> Philip McGrath <philip@philipmcgrath.com> writes:
>  >>
>  >>> If we took your final suggestion above, I think we'd have
> something
>  >>> like this:
>  >>>
>  >>> ```
>  >>> #:phases
>  >>> (modify-phases %standard-phases
>  >>>    (add-after 'unpack 'delete-dependencies
>  >>>      (make-delete-dependencies-phase '("node-tap"))))
>  >>> ```
>  >>
>  >> I’m perfectly happy with this if it’s a compromise we all can
> agree on.
>  >> It is exactly what popped into my imagination when I read
> Liliana’s
>  >> suggestion.  I guess the one thing missing is that it would not
>  >> necessarily be implemented on top of better “package.json”
>  >> manipulation support.  That said, it doesn’t preclude providing
> that
>  >> support if/when the need arises.
>  > In my personal opinion, we would write that support first and
> perhaps
>  > the shorthands later.  I.e.
>  >
>  > (add-after 'patch-dependencies 'drop-junk
>  >    (lambda _
>  >      (with-atomic-json-replacement "package.json"
>  >        (lambda (json) (delete-dependencies json '("node-tap"))))))
To be fair, finding the right sweet spot between being overly verbose
and code golfing is difficult.

> Certainly I do agree that it would be better to support code more 
> concise than that! But I think all of these variations are strictly 
> worse than '#:absent-dependencies'. It isn't just that they are more 
> typing: the require authors of package definitions to have some (not 
> very much, but some) procedural knowledge about _how_ 'node-build-
> system' deals with the absence of dependencies, rather
> than with '#:absent-dependencies', declaratively specifying _what_ is
> to be done. 
Understanding build systems is for nerds.  We here at leftpad.org care
about the things that are really important.

> For example, as I mentioned in my cover letter at 
> <https://issues.guix.gnu.org/51838#257>, even my own code from the 
> exchange I just quoted:
> 
>  >>>    (add-after 'unpack 'delete-dependencies
>  >>>      (make-delete-dependencies-phase '("node-tap"))))
> 
> would be broken in v6, because the implementation of 
> 'delete-dependencies' assumes that the 'patch-dependencies' phase has
> already been run. 
I think that is an issue with your patch, however.  With mine, you
could at least add "peerDependencies" on your own.

> I think this is an implementation detail that users of 
> 'node-build-system' should not be required to know! Indeed, I think
> that would be a good reason to have 'patch-dependencies' handle the
> absent dependencies itself [...]
I think something like that can be arranged.  We could make patch-
dependencies drop all the dependencies it doesn't know about if we add
a "verify-dependencies" phase before it.  (Note that I already
suggested that once).  verify-dependencies would raise an error if a
dependency is missing and the user could then decide to drop it or
(add-before 'verify-dependencies 'drop-dependencies ...).

> I expect the majority of Guix's Node.js packages will continue for
> the foreseeable future to need the functionality for absent
> dependencies I described at the beginning of this email, so I think
> we should provide it through a mechanism that is as high-level,
> concise, declarative as possible, and ideally one that will
> facilitate automated code generation and static analysis. On each of
> these criteria, I think '#:absent-dependencies' is better than any of
> the other proposals I've heard.
> 
> But, as I said, in the interest of compromise and moving forward, I'm
> willing to live with something based on v6 for now if that's what can
> achieve consensus, and then propose '#:absent-dependencies'
> separately.  So, if you want me to send a new version with one of
> these other variations, tell me which one, and I'll do it.
I think amending v6 with what we learn from this discussion and the
discussion on patch 3 of this series is the way to go.

Another thing I forget to mention all the time are regexps.  I think
it'd be beneficial if delete-dependencies could delete dependencies
based on their name matching a regexp rather than a string exactly. 
This would make some of your lists shorter (e.g. "karma.*"), but there
might be a debate on whether to use "^karma.*$" or whether to only
consider regexps that match the dependency fully.

> I hope my tone isn't coming across the wrong way---I really don't
> mean to be snarky! But I am genuinely struggling to understand the 
> significance of the difference between:
> 
>  >>>    (add-after 'unpack 'delete-dependencies
>  >>>      (make-delete-dependencies-phase '("node-tap"))))
> 
> which I thought you objected to, and the result of what I think
> you've most recently proposed:
> 
>      (add-after 'patch-dependencies 'delete-dependencies
>        (lambda () (delete-dependencies '("node-tap"))))
To be clear, there are two things in here which I objected.
1. make-delete-dependencies-phase as a procedure which returns the
actual phase instead of writing the lambda out.  BTW if I ever wrote
(lambda () (delete-dependencies ...)), that was a mistake on my part.
As for the reason, scroll up.
2. implementing delete-dependencies in terms of messy ad-hoc json
primitives using inner defines rather than reusable ones.

The goal was not to create the most unwieldy incantation even though it
certainly appears as though it was looking back.  Sorry for the
misunderstanding.

> which would have avoided my earlier reservations about making the
> JSON representation part of the build system's public API for the
> first time.
> 
> So I'm not feeling very confident in my ability to predict what
> changes would or would not block consensus.
Adding a gratuitous keyword is an immediate blocker as we've discussed
at length ;)
Using curried functions is also unlikely going to meaningfully advance
the discussion, but there could be a niche application in which they're
actually useful that I had not thought about.

Apart from that, there is a large room simply for discussions, where it
is unclear what form consensus will take before it is shaped.  For
instance, for patch 3, you had to write a lot of functionality from
scratch, which prompted me to suggest that you put them in their own
file (which would then be public API, though), but with a little bit of
refactoring in terms of SRFI-1 we might make it small enough to keep to
npm-build-system until Rust folks demand the same goodies.  Phase
ordering as you mentioned is also still up to debate.  So there's a lot
in which you can can add your opinion.

If you are doubtful about whether or not a particular change would be
good, you could probe different versions you have in your head and ask
for opinions on them.  E.g. provide two or three implementations of
assoc-set and let the "best" be selected.  If you only have a single
one, you can still ask if something like that would be okay and if you
receive a "No" that you don't understand it's okay to ask "what would
be the problem, what solution do you propose?" etc.  XKCD's "inventing
a new standard to cover both use-cases" applies as always too :)

Cheers

Hi,

On 12/30/21 21:46, Liliana Marie Prikler wrote:
> Am Donnerstag, dem 30.12.2021 um 20:09 -0500 schrieb Philip McGrath:
>> I still feel that there's something I'm fundamentally not
>> understanding about your objections to '#:make-absent-dependencies',
>> which is why, in v6, I tried to do exactly as you had proposed:
>>
>> On 12/20/21 17:00, Liliana Marie Prikler wrote:
>>   > Hi Timothy,
>>   >
>>   > Am Montag, dem 20.12.2021 um 15:15 -0500 schrieb Timothy Sample:
>>   >> Hi Philip,
>>   >>
>>   >> Philip McGrath <philip@philipmcgrath.com> writes:
>>   >>
>>   >>> If we took your final suggestion above, I think we'd have
>> something
>>   >>> like this:
>>   >>>
>>   >>> ```
>>   >>> #:phases
>>   >>> (modify-phases %standard-phases
>>   >>>    (add-after 'unpack 'delete-dependencies
>>   >>>      (make-delete-dependencies-phase '("node-tap"))))
>>   >>> ```
>>   >>
>>   >> I’m perfectly happy with this if it’s a compromise we all can
>> agree on.
>>   >> It is exactly what popped into my imagination when I read
>> Liliana’s
>>   >> suggestion.  I guess the one thing missing is that it would not
>>   >> necessarily be implemented on top of better “package.json”
>>   >> manipulation support.  That said, it doesn’t preclude providing
>> that
>>   >> support if/when the need arises.
>>   > In my personal opinion, we would write that support first and
>> perhaps
>>   > the shorthands later.  I.e.
>>   >
>>   > (add-after 'patch-dependencies 'drop-junk
>>   >    (lambda _
>>   >      (with-atomic-json-replacement "package.json"
>>   >        (lambda (json) (delete-dependencies json '("node-tap"))))))
> To be fair, finding the right sweet spot between being overly verbose
> and code golfing is difficult.

I will admit that I am more than a little frustrated that, having put 
aside my own reservations and implemented the compromise proposal it 
seemed everyone could live with, it now seems that the consensus was in 
fact illusory. Moreover, I still do not understand what specific changes 
I could send in a v8 that would get this patch series to a state where 
everyone would agree it could be applied.

> 
>> Certainly I do agree that it would be better to support code more
>> concise than that! But I think all of these variations are strictly
>> worse than '#:absent-dependencies'. It isn't just that they are more
>> typing: the require authors of package definitions to have some (not
>> very much, but some) procedural knowledge about _how_ 'node-build-
>> system' deals with the absence of dependencies, rather
>> than with '#:absent-dependencies', declaratively specifying _what_ is
>> to be done.
> Understanding build systems is for nerds.  We here at leftpad.org care
> about the things that are really important.
> 

I don't know what to make of this comment, and I especially don't 
understand what the notorious left-pad incident has to do with my views. 
Aside from its inflammatory nature, I think left-pad is a confusing way 
to make a point because many people have tried to make it "stand for" 
many different, perhaps even mutually contradictory, conclusions.

In case it helps at all to state my position more fully: with or without 
Guix, I think a major purpose, perhaps even the primary purpose, of 
_any_ build system is to relieve users (including ourselves) of the 
cognitive burden of lower-level details. Build systems are a means of 
abstraction and encapsulation.

We use './configure && make && make install' to avoid having to know 
about the intricacies of flags for gcc, ordering requirements for 
building sub-projects, and innumerable quirks of strange and venerable 
systems that Autotools supports so that most of us can ignore them. We 
use 'gnu-build-system' to avoid having to know, and constantly repeat, 
the details of invoking 'configure' and 'make' to find Guix inputs and 
build to Guix outputs, along with further details of what files need to 
be patched when. Sometimes we nonetheless need to drop down to a lower 
level of abstraction, and Guix makes it convenient to do so, but the 
goal is to provide useful abstractions.

For a personal example, I've never written so much as "Hello, world" in 
Go, but I've been able to contribute packages to Guix for software I use 
that happens to be written in Go, which was much easier to do because 
'go-build-system' offers useful high-level abstractions like 
'#:unpack-path' and '#:import-path'. I didn't need to know _how_ 
'go-build-system' arranges the build environment based on those values, 
or even anything beyond the most superficial understanding of _what_ 
those values are, in order to build a Go package.

>> which would have avoided my earlier reservations about making the
>> JSON representation part of the build system's public API for the
>> first time.
>>
>> So I'm not feeling very confident in my ability to predict what
>> changes would or would not block consensus.
> Adding a gratuitous keyword is an immediate blocker as we've discussed
> at length ;)

Even when I have disagreed with your point of view, I have been trying 
my best to understand it, and I don't doubt that it is offered in good 
faith. I hope this is just a matter of some nuance in the connotation of 
the word "gratuitous" not coming across properly, but I would appreciate 
the same consideration being extended to my perspective.

Almost tautologically, I don't think adding '#:absent-dependencies' 
would be gratuitous, or I wouldn't have proposed it.

I don't want to speak for anyone but myself, but I haven't heard anyone 
else share these objections. And again, I also agreed to v6 of this 
series, in which I removed the keyword.

I don't have time right now to go through the latest comments point by 
point: I will try to do so later. From my perspective, though, the 
substance of these patches has been ready to go for something like six 
weeks now, and the few enhancements since then could easily have been 
small follow-on patches. I would consider it very regrettable if this 
patch series were to continue to be blocked by stylistic considerations 
in the implementation of unexported helper functions.

-Philip

On Wed, Jan 05, 2022 at 02:08:30PM -0500, Philip McGrath wrote:
> I would consider it very regrettable if this patch series were to
> continue to be blocked by stylistic considerations in the implementation of
> unexported helper functions.

Agreed.

Is there a concrete problem with these patches? Or will they work as
specified for Guix packagers?

Let's remember that the primary goal of code review is to bring a
contribution into the codebase.

We have added suboptimal code to Guix many times, because it worked well
enough. Later we can refine things.

You mention go-build-system as being useful for you. That's very
gratifying, because it was a lot of hard work for me to help finalize
those patches, and they were quite far from ideal even when they were
committed. But the build system allowed Guix users to add Go packages,
which later attracted more contributions, and the go-build-system keeps
improving as a result.

Hi,

Am Mittwoch, dem 05.01.2022 um 14:08 -0500 schrieb Philip McGrath:
> I will admit that I am more than a little frustrated that, having put
> aside my own reservations and implemented the compromise proposal it 
> seemed everyone could live with, it now seems that the consensus was
> in fact illusory. Moreover, I still do not understand what specific
> changes I could send in a v8 that would get this patch series to a
> state where everyone would agree it could be applied.
To be honest, I should do part of my job as well here and apply some of
my own suggestions as I push if you deem them reasonable.  I think
that'd get us to consensus in one iteration.

> 
> I don't know what to make of this comment, and I especially don't 
> understand what the notorious left-pad incident has to do with my
> views.  Aside from its inflammatory nature, I think left-pad is a
> confusing way to make a point because many people have tried to make
> it "stand for" many different, perhaps even mutually contradictory,
> conclusions.
Point taken, I'll try to dedent my Javascript jokes.

> In case it helps at all to state my position more fully: with or
> without Guix, I think a major purpose, perhaps even the primary
> purpose, of _any_ build system is to relieve users (including
> ourselves) of the cognitive burden of lower-level details. Build
> systems are a means of abstraction and encapsulation.
> 
> [...]
I agree with you that abstractions ought to help, but we do have some
disagreements about the amount by which certain abstractions help. 
Those are gut feeling value judgements, they're not all entirely
rational.

> I hope this is just a matter of some nuance in the connotation of 
> the word "gratuitous" not coming across properly, but I would
> appreciate the same consideration being extended to my perspective.
> 
> Almost tautologically, I don't think adding '#:absent-dependencies' 
> would be gratuitous, or I wouldn't have proposed it.
Generally, keywords are reserved for a few special operations.  I don't
currently have the time to write them all up, but suffice it to say I
don't believe the way #:absent-dependencies would be used fits into any
of those.  I can write that up in a later message if you feel it's
imporant enough.

> From my perspective, though, the substance of these patches has been
> ready to go for something like six weeks now, and the few
> enhancements since then could easily have been small follow-on
> patches. I would consider it very regrettable if this patch series
> were to continue to be blocked by stylistic considerations 
> in the implementation of unexported helper functions.
That's not at all my intention.  I'll write up a checklist as a follow-
up.  We can go over these shortly and I'll have v6 with minor
adjustments land hopefully soon (if it can't be done tomorrow I'll
shoot for Sunday).  Would that be acceptable?

Am Mittwoch, dem 05.01.2022 um 22:04 +0100 schrieb Liliana Marie
Prikler:
> That's not at all my intention.  I'll write up a checklist as a
> follow-up.  We can go over these shortly and I'll have v6 with minor
> adjustments land hopefully soon (if it can't be done tomorrow I'll
> shoot for Sunday).  Would that be acceptable?
Okay, here's my checklist.  I'll denote discussion points with numbers
and actionable tasks with square brackets.  Send me a reply in which
you mark either the ones you want to have or the ones you don't (give a
short reason for each) and I'll make it work.  The entries marked with
a forward slash would require an additional round of review to ensure I
don't fuck any of this up (pardon my lack of a better term), so it's
probably better to open up separate bugs for those if we want to be
quick.  I feel rather confident about the rest, however.

1. Move alist/json primitives to new file or shrink them in vertical
size.  Since you raised valid points against moving them, that'd be
shrinking them in vertical size.  To this end:
[ ] Use SRFI-71 wherever applicable.
[ ] Implement them in terms of SRFI-1 span.
[ ] For the json-side only define json-update* as it appears to be the
only one used later on (need confirmation on that one)

2. Reduce Racketisms
[ ] Use DEFAULT instead of FAILURE_RESULT.
[/] Define json-update* as syntax.

3. Generic style stuff
[ ] Move conses inside match.
[/] Merge with-atomic-json-file-replacement and json-update into a
single syntax (rewrite-json-file).
[ ] Simplify delete-dependencies so that we can directly write 
(lambda args (delete-dependencies DEPS).
[ ] Use identifier-syntax for the empty JSON object (how do we name
it?)

If I omitted anything and it's simple enough to fit this mold, do add
it and I'll see if I can tack it on or we would need to go through
another round for that.  Sorry this all took so long.

Hi,

On 1/5/22 16:04, Liliana Marie Prikler wrote:
> Am Mittwoch, dem 05.01.2022 um 14:08 -0500 schrieb Philip McGrath:
>> In case it helps at all to state my position more fully: with or
>> without Guix, I think a major purpose, perhaps even the primary
>> purpose, of _any_ build system is to relieve users (including
>> ourselves) of the cognitive burden of lower-level details. Build
>> systems are a means of abstraction and encapsulation.
>>
>> [...]
> I agree with you that abstractions ought to help, but we do have some
> disagreements about the amount by which certain abstractions help.
> Those are gut feeling value judgements, they're not all entirely
> rational.
> 
>> I hope this is just a matter of some nuance in the connotation of
>> the word "gratuitous" not coming across properly, but I would
>> appreciate the same consideration being extended to my perspective.
>>
>> Almost tautologically, I don't think adding '#:absent-dependencies'
>> would be gratuitous, or I wouldn't have proposed it.
> Generally, keywords are reserved for a few special operations.  I don't
> currently have the time to write them all up, but suffice it to say I
> don't believe the way #:absent-dependencies would be used fits into any
> of those.  I can write that up in a later message if you feel it's
> imporant enough.

It isn't needed right now, since we've agreed to go ahead without 
'#:absent-dependencies', but, since I do intend to propose 
'#:absent-dependencies' immediately thereafter, I think it would be 
useful: this seems to get close to the core of the disagreement we've 
been having for the last ... couple months?

I don't see a reason why we should hesitate to use keywords when they 
enable especially nice code. Actually, I've sometimes wished build 
systems would '#:allow-other-keys'.

I'd expect '#:absent-dependencies' to be more common for 
'node-build-system' packages than '#:tests?', since I'd expect almost 
every package that would use '#:tests? #f', plus a significant number 
that wouldn't, to use '#:absent-dependencies'.

Jumping back to an earlier email:

On 12/30/21 21:46, Liliana Marie Prikler wrote:
 > Am Donnerstag, dem 30.12.2021 um 20:09 -0500 schrieb Philip McGrath:
 >> In my view, the high-level purpose of 'delete-dependencies',
 >> '#:absent-dependencies', or whatever is to gather our collective
 >> procedural knowledge about how to modify a "package.json" file to
 >> build a package without some of the dependencies its developers have
 >> declared and to encode that knowledge in a single, abstracted point
 >> of control in 'node-build-system', so that authors of Node.js package
 >> definitions can simply specify which declared dependencies are absent
 >> and leave it to 'node-build-system' to act accordingly. (I don't
 >> think it matters _why_ the dependencies are absent, i.e. whether we
 >> don't want the them or merely don't have them.)
 > For the record, you can delete present dependencies as well, which is
 > one shortcoming in the "absent dependencies" metaphor.
[...]

 >> It is unnecessarily flexible because, if a package author ever passes
 >> some other value for '#:json-keys', that would seem to indicate that
 >> there's some procedural knowledge missing from 'node-build-system',
 >> and it should be added there.
 > The reason I have it as such is that a packager might decide in the
 > future to e.g. only drop a peer dependency, that might share a name
 > with a dev dependency or something along those lines.

Since I don't think I've written it down before, my hope in describing 
them as "absent" dependencies was to state that they are absent from the 
build environment, while being as neutral as I could about _why_ they 
might be absent. In particular, I wanted to avoid the implications of 
"missing", which can have implications of "we don't know where X is" and 
"it would be better if we found X". One day, it would probably be nice 
to have 'node-aws-sdk' packaged for Guix, but, even if we knew the 
precise line number in "node-xyz.scm" where it we could find its 
definition, we would not want to use it while building 'node-sqlite3'.

So, having established that the adjectives are a little fuzzy, I don't 
understand what it would mean to "delete present dependencies". If they 
are present in the build environment, why delete them? If the issue is 
that they only are needed at build time, the 'install' phase of 
'node-build-system' should already handle this by passing '--production' 
to 'npm install'.

If a "peerDependency" and a "devDependency" share a name, then they 
refer to the same package. I believe it would be an error (logically, 
that is: I do know npm would not raise an exception) to have a 
"peerDependency" that is also a "devDependency" or a "dependency": as I 
understand it (poorly!), peer dependencies are meant to be some weaker 
kind of relationship.

But again, none of this needs to stand in the way of merging this patch 
series.

-Philip

Hi,

Am Freitag, dem 07.01.2022 um 23:14 -0500 schrieb Philip McGrath:
> I do intend to propose  '#:absent-dependencies' immediately
> thereafter.
And I do intend to reject that proposal immediately thereafter. 
Nothing personal, but I think I've made a clear enough case against it,
although I could go into even more detail if you want me to.

> I don't see a reason why we should hesitate to use keywords when they
> enable especially nice code. Actually, I've sometimes wished build 
> systems would '#:allow-other-keys'.
The reason to e.g. use #:tests? over (delete 'check) is not because the
code looks nicer (although it does).  As Jelle pointed out, code that
does ugly things should be allowed to look ugly, and in terms of node-
build-system needing to delete dependencies *at all* is already an ugly
thing.  There's no need for a convenience keyword, much less a reason
to implement one for the sake of nicer-looking code.

> Since I don't think I've written it down before, my hope in
> describing them as "absent" dependencies was to state that they are
> absent from the build environment, while being as neutral as I could
> about _why_ they might be absent. 
The word absent is not neutral.  It implies it's not there when it
should be.  My use of unwanted does the opposite.  It implies it should
not be there when upstream claims it should.  The obvious middle ground
is good old "exclude", but I think it'd be hard to make an argument
even for #:excluded-dependencies.

> In particular, I wanted to avoid the implications of
> "missing", which can have implications of "we don't know where X is"
> and "it would be better if we found X". 
You just substituted missing for another spelling of it.  That doesn't
really help here.

> So, having established that the adjectives are a little fuzzy, I
> don't understand what it would mean to "delete present dependencies".
> If they are present in the build environment, why delete them? 
It would mean committing the error of specifying a dependency both as
input and as absent.  This is for the lack of a better word UB.


> If a "peerDependency" and a "devDependency" share a name, then they 
> refer to the same package. I believe it would be an error (logically,
> that is: I do know npm would not raise an exception) to have a 
> "peerDependency" that is also a "devDependency" or a "dependency": as
> I understand it (poorly!), peer dependencies are meant to be some
> weaker kind of relationship.
Even then, much of our other discussion revolves around the question of
what the implications of those are.  If we, the experts, can't be
trusted to have a clear enough understanding, how should we trust non-
expert users on the matter?  That's why I wanted to encode the
dependency key in delete-dependencies.  That way, one could specify
"delete X from dependencies after it was introduced by Guix" or "no, I
really don't want it to be a peer dependency either".  And until we
have a clearer image, we could accept both forms and see what problems
they'd cause later on, then reject one in favour of the other by
patching them out over time.

None of that would be possible with a keyword, at least one with a nice
value encoding.  You either change all node packages at a time,
potentially causing a c-u-worthy rebuild, or you don't and you're
stuck.

Cheers