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

[Philip McGrath <philip@philipmcgrath.com>]

Hi,

On 12/18/21 15:49, Liliana Marie Prikler wrote:
> Hi,
> 
> Am Samstag, dem 18.12.2021 um 13:31 -0500 schrieb Philip McGrath:
>> I also feel like I'm missing something, though maybe I just disagree.
>>
>> To try to be concrete, here's a real example. Between v3 and v4 of
>> this patch series, I discovered that leaving out node-debug could
>> actually cause runtime problems for some of the node-serialport
>> packages. (It turns out it's really a logging library, which wasn't
>> what I'd thought at first.) After I added the node-debug, I could
>> easily search node-xyz.scm for the packages that listed it among
>> their #:absent-dependencies and add it to them as an input.
> Wouldn't we get a huge stack trace of doom with the failed require in
> that case if it was truly non-negotiable, though?
> 
>> It seems like this would be much less convenient if node-build-system
>> were to silently delete such dependencies and simply print a warning.
>> I guess I would have to search through the build logs for all Node.js
>> packages, right?
> Since node packages do have the tendency to pull in the entire
> internet, you might not be too far off with that statement, but again
> if you have a particular issue in a single package due to an omitted
> dependency, the offending package ought to be close to the most recent
> call on the stack.

I think this is part of where our expectations are diverging, because I 
think the failure mode for CommonJS require (ES6 import may be at least 
somewhat better) is much less clean than this seems to suggest.

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.

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.

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

> 
> An alternative to searching through the build logs would also be to
> build all the sources and grepping their package.jsons ;)
> 
>> More generally, I think truly "optional dependencies" (in the Node.js
>> sense, to the extent I understand it) and dependencies we actively
>> don't want (e.g. because node-sqlite3 shouldn't transitively depend
>> on node-aws-sdk) are relatively rare cases.
>>
>> The most common cases seem to be dependencies we really would like to
>> have, such as test frameworks or Typescript, but haven't packaged
>> yet, and thus need to work around. Many packages that have
>> #:absent-dependencies for such reasons also need to use `#:tests? #f`
>> or to replace the build phase with some kind of manual alternative.
>>
>> I guess I don't understand what case the warn-and-drop approach is
>> optimizing for. For both the case when dependencies aren't packaged
>> for Guix yet and the case when Guix packagers have actively decided
>> to skip some upstream dependencies, I think #:absent-dependencies is
>> more useful. Having to look for that information in warnings in the
>> build log seems much less ergonomic.
> That's why my original suggestion was to have a switch between "strict"
> meaning we fail as before and "warn" meaning "we know we have unmet
> dependencies".  The "warn" case would be annotated similar to #:tests?
> #f and the strict case default.  So you could still communicate
> important missing packages like typescript et al., but people who hack
> on their own channels with lower standards can wing it without having
> to delete configure.

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

-- 
2.32.0


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


Would that meet your objective?

>> Also, currently node-build-system doesn't seem to be removing those
>> files which `npm pack` is supposed to exclude, which would probably
>> be a  prerequisite for addressing this.
> For the record, which files would that be?  Could we do emacs-build-
> system style #:include and #:exclude lists?

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.


>> I think there is room for improvement in node-build-system. One thing
>> I've been thinking about is some articles I've seen (but not fully
>> thought through yet) from the developers of PNPM, an alternative
>> package manager for Node.js, which seems to have some similarities to
>> Guix in symlinking things to a "store".[1][2][3] (It could be
>> especially interesting for bootstrapping npm! And the approach to
>> "monorepos" also seems relevant.) I also think an importer is very
>> important, even if it's an imperfect one: `guix import npm-binary`
>> was indispensable in developing these patches. I have some ideas
>> about improving it, in particular that we should assume the newer "^"
>> semantics for dependencies everywhere (i.e. that major versions and
>> only major versions have incompatible changes: a common case recently
>> seems to be moving from CommonJS modules to ES6 modules).
>>
>> As I understand it, node-build-system is undocumented, with no
>> guarantees of compatibility. If #:absent-dependencies is at least an
>> improvement over the status quo---which I think it is, since the new
>> packages wouldn't build with the status quo---could we apply this and
>> replace it later if someone implements a better strategy? I don't
>> think I can implement control-flow analysis for JavaScript within the
>> scope of this patch series.
> It's sadly not that easy.  See XKCD 1172.

Certainly that's true in general, but I thought this warning from [3] 
would apply here:

     When you maintain package definitions outside Guix, we, Guix
     developers, consider that *the compatibility burden is on you.*
     Remember that package modules and package definitions are just
     Scheme code that uses various programming interfaces (APIs). We
     want to remain free to change these APIs to keep improving Guix,
     possibly in ways that break your channel. We never change APIs
     gratuitously, but we will *not* commit to freezing APIs either.

-Philip

[1]: https://webthings.io/
[2]: https://gitlab.com/philip1/guix-patches/-/tree/wip-webthings
[3]: https://guix.gnu.org/manual/devel/en/html_node/Creating-a-Channel.html