[bug#51838] [PATCH v5 07/45] guix: node-build-system: Add #:absent-dependencies argument.

[Philip McGrath <philip@philipmcgrath.com>]

Hi,

On 12/18/21 20:02, Liliana Marie Prikler wrote:
> Am Samstag, dem 18.12.2021 um 17:55 -0500 schrieb Philip McGrath:
>> somewhat, but it was immensely helpful to be able to find in node-
>> xyz.scm all of the packages that wanted, but did not have, node-debug.
>> I imagine it would be even more useful in a more tangled dependency
>> graph.
> That might be beneficial in your particular use case here, but you also
> have to think about the maintenance burden your mechanism introduces.
> What if another leftpad happens and we have to speedrun our core-
> updates cycle to add #:absent-dependencies everywhere?

I don't understand the scenario you have in mind here.

As I remember/understand the left-pad debacle, the package was deleted 
from the NPM registry by its author. I don't understand how that would 
require changes to Guix package definitions. Hopefully, Software 
Heritage would save the day. Perhaps Guix would choose to provide an 
alternate, compatible package implementing that single, trivial 
function, but we could do that by just changing the definition of the 
hypothetical Scheme variable `node-left-pad`. Eventually, downstream 
packages would presumably make changes, but we'd have to address those 
changes anyway when updating those packages.

Even if I assume a scenario that is going to be fixed by removing a 
hypothetical `node-left-pad` packages from all the packages to which it 
is an input, that would change O(n) lines of code: having to change 
#:absent-dependencies in all cases would just be O(2n) lines, which 
doesn't seem prohibitive. Indeed, I think I would be glad to have an 
entry in #:absent-dependencies in such a case, communicating explicitly 
in the Guix repository that the package was affected and Guix packagers 
adopted a workaround. If I'm later updating the package, I can check to 
see if the workaround is still needed or if upstream has dropped 
node-left-pad. In any case, I much prefer to have this written 
explicitly in code in the Guix repository, rather than relying on 
external mechanisms like build logs and upstream source files.

Additionally, I think the use case I encountered with node-debug is 
likely to come up fairly often. For example, someone might notice that a 
lot of packages use `node-tap` for testing, package it for Guix, and 
then want to find all of the downstream packages to which to add it and 
enable tests.

>> I think we should optimize for the kind of high-quality packages we
>> aspire to have in mainline Guix, where eventually we hope to have all
>> sufficiently useful libre Node.js packages available. In that case,
>> #:absent-dependencies makes it explicit when Guix packagers are
>> overriding an upstream decision. While we work to achieve that reality,
>> I think #:absent-dependencies is a much better place for a to-do list
>> than having to search build logs or source "package.json"s. However...
> Compare this to tests.  We have a keyword to disable all tests, which
> defaults to #f and we have other idioms for disabling particular tests.
> Your use case is no different.  If at all, we should only have a
> keyword to disable the check completely and other idioms to e.g. patch
> the package.json file so that sanity-check/patch-dependencies/what-
> have-you doesn't error when it relies on its contents.

I don't mean to be dense, but I feel like I'm missing something. I 
assume the reason we don't have a declarative, high-level mechanism for 
disabling specific tests is that there isn't a general convention for 
doing that, AFAIK. We do have `#:configure-flags`, which can be used to 
pass things like `--disable-whatever`, even though, in principle, that 
could be done by replacing the configure phase. I see 
#:absent-dependencies as similar: it provides, IMO, a readable, 
declarative mechanism to make a commonly-needed adjustment to the 
behavior of the patch-dependencies phase.

>> I can see the use of a "warn" mode for hacking things together quickly
>> without having to totally delete configure. I think this could coexist
>> with #:absent-dependencies and could be done in a follow-on to this
>> patch series.

>> --8<---------------cut here---------------start------------->8---
>> (if (or (member key absent-dependencies)
>>           (and (memq 'infer absent-dependencies)
>>                (not (hash-ref index key #f))))
>>       acc
>>       (acons key (hash-ref index key value) acc))
>> --8<---------------cut here---------------end--------------->8---
> You're actively making the code that resolves dependencies worse to
> look at only to keep the keyword.  Don't.  There are tools besides a
> hammer.
> 
>> Would that meet your objective?
> No.  As I repeatedly pointed out, I want either no keyword addition for
> this "feature" or a keyword that can be regarded as essentially boolean
> if not outright implemented as one.
> 
> Reading this again, the existing lines already do what I want (hash-ref
> index key value) spits out value as a default if key is not found.  So
> the solution is to not touch patch-dependencies at all; we don't have
> to abuse a function that's not meant for sanity checking to check
> sanity.

To clarify, I thought you wanted `node-build-system` to issue a warning 
and drop dependencies not supplied as package inputs. Is that correct? 
In the existing code, if `key` is not found and `value` is returned, the 
configure phase (i.e. `npm install`) will always fail. (The name 
`patch-dependencies` may be a little vague about the actual purpose of 
this phase.)

>> We never change APIs gratuitously.
> In my personal opinion, #:absent-dependencies would be a gratuitous
> change in API.  There's no need to have this in the build system.  We
> should rather look into ways that make it possible/easy for users to
> patch the package.json file between unpack and configure.

I don't think adding #:absent-dependencies is a breaking change in the 
API at all. Any package that builds currently should continue to build 
with #:absent-dependencies support added, and indeed with this entire 
patch series.

> This also calls back to my earlier point of the assoc-set! being out of
> place, which itself is also a manifestation of node-build-system not
> exposing adequate primitives towards rewriting essential files.  For
> instance, the procedure
> 
>    (lambda (file proc)
>      (with-atomic-file-replacement file
>        (lambda (in out)
>          (write-json (proc (read-json in))))))
> 
> would probably deserve its own name and export from node-build-system.
> Yes, you can export utility procedures from (guix build *-build-
> system), look at the Emacs and Python build systems for example.

I do agree that we should provide more utilities for transforming 
"package.json" in general ways. It would be nice to make such 
transformations at least as convenient as more fragile ones using 
`substitute*`. But, as I wrote earlier, that seems out of scope for this 
patch series.

> 
> With this in place, we both get to have our cakes and eat it too.
> There would be no keyword added, and package maintainers would drop
> "absent" dependencies in a phase like so
> 
>    (add-after 'unpack 'drop-dependencies
>      (lambda _
>        (with-atomic-json-replacement "package.json"
>          (lambda (json)
>            (map (match-lambda
>                   (('dependencies '@ . DEPENDENCIES)
>                    (filter away unwanted dependencies))
>                   (('devDependencies '@ . DEPENDENCIES)
>                    (same))
>                   (otherwise otherwise))
>                 json)))))
> 
> Of course, if that's too verbose, you can also expose that as a
> function from node-build-system.  Then all we'd have to bikeshed is the
> name, which would be comparatively simple.
> 
> Does that sound like a reasonable plan to you?

I'm not sure how to proceed here.

I still think the #:absent-dependencies keyword, with or without a 
"warn" mode, is the best approach. It has sounded like others on this 
thread also liked the approach, though I don't want to speak for anyone 
but myself.

I can understand that you would prefer a different approach. I can see 
how a warn-and-ignore could be useful in some cases. My last proposal 
was an attempt at a compromise, showing how adding #:absent-dependencies 
would not preclude adding support for a warn-and-ignore mode later.

But the impression I'm getting is that you think the 
#:absent-dependencies approach would be not merely sub-optimal but 
actively harmful, and, if that is indeed your view, I feel like I'm 
still failing to understand what the harm is.

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"))))
```

That seems pretty similar to, under the current patch series:

```
#:absent-dependencies '("node-tap")
```

I can see pros and cons to both approaches. I still like 
`#:absent-dependencies` better, as I find it less verbose, more 
declarative, ... trade-offs we probably don't need to rehash further. 
But it sounds like you see a huge, prohibitive downside to 
`#:absent-dependencies`, and I am just not managing to see what that is.

I don't know what further steps to take to resolve this disagreement or 
how some decision ultimately gets made.

More broadly, I agree with you that the current `node-build-system` has 
some ugly code and is missing some useful utility functions. But I don't 
feel like I can address all of those preexisting issues in the scope of 
this patch series, which has already become somewhat unwieldy.

Maybe someone else could weigh in on how to proceed?

-Philip