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

[Liliana Marie Prikler]

Am Samstag, dem 18.12.2021 um 17:55 -0500 schrieb Philip McGrath:
> [T]he failure mode for CommonJS require (ES6 import may be at least 
> somewhat better) is much less clean than this seems to suggest.
I think the less I know about JavaScript, the better I'll sleep at
night.

> Returning to my concrete example, my motivation for this has been
> trying to set up a Guix System service to run the Webthings Gateway
> (formerly Mozilla IoT).[1] (Currently I have it on a branch of my Guix
> fork at [2]; since there are many yacks left to shave before it could
> be upstreamed, I plan to turn it into a channel once it can build
> against Guix master, i.e. once this patch series is applied.) I
> discovered the problem with the missing node-debug only when webthings-
> service caused webthings-gateway to load webthings-addon-zwave-adapter
> at runtime. 
> (Those are both node-build-system packages, but there are also 
> webthings-addon-* packages in Python and Rust, and in principle any 
> language could be used.) The webthings-addon-zwave-adapter has 
> node-serialport as a package.json dependency. As I remember it, even 
> then, the lack of node-debug only caused a runtime error on a certain
> code path, perhaps triggered by the presence or absence of Z-Wave 
> adapter hardware: I had used all of the packages without problems
> before hitting the issue. There was a stack trace, and it did help
> 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?

> In particular, we don't have any JavaScript test frameworks packaged
> for Guix yet, so most of the time we aren't actually running the code
> in our Node.js packages, just putting the right files in the right
> places. So the runtime error may not come until some application/tool
> has been created, potentially very far along the dependency graph, and
> may not be revealed by tests (that we can actually run) at all.
The fix to not having test frameworks is adding them, not 

> 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 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.
> 
> Right now, the #:absent-dependencies argument must be a list of
> strings. To support a "warn" mode, we could loosen that contract a bit:
> we can bikeshed about details, but let's say that we're in "warn" mode
> if the list contains the symbol 'infer. We change this code:
> 
> ---
> 
> diff --git a/guix/build/node-build-system.scm 
> b/guix/build/node-build-system.scm
> index b74e593838..892104b6d2 100644
> --- a/guix/build/node-build-system.scm
> +++ b/guix/build/node-build-system.scm
> @@ -69,7 +69,8 @@ (define (list-modules directory)
>                 input-paths)
>       index))
> 
> -(define* (patch-dependencies #:key inputs #:allow-other-keys)
> +(define* (patch-dependencies #:key inputs absent-dependencies
> +                             #:allow-other-keys)
> 
>     (define index (index-modules (map cdr inputs)))
> 
> @@ -86,7 +87,9 @@ (define (resolve-dependencies meta-alist meta-key)
>         (('@ . orig-deps)
>          (fold (match-lambda*
>                  (((key . value) acc)
> -                (acons key (hash-ref index key value) acc)))
> +                (if (member key absent-dependencies)
> +                    acc
> +                    (acons key (hash-ref index key value) acc))))
>                '()
>                orig-deps))))
> 
> to do something like this:
> 
> --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.

> It at least includes files listed in ".npmignore", but I think there
> are entries in "package.json" that control the behavior, too. We should
> adjust our repack phase to ignore those files---but I, at least, would
> need to look into it further to know exactly what the correct behavior
> is.
Fair enough, that is probably out of scope for now, but we should
revisit it before importing the node world and changing node-build-
system becomes a core-updates task.

> 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.

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.

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?