Package: guix-patches;
Reported by: Philip McGrath <philip <at> philipmcgrath.com>
Date: Sun, 14 Nov 2021 12:43:01 UTC
Severity: normal
Tags: patch
Done: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Philip McGrath <philip <at> philipmcgrath.com> To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>, Timothy Sample <samplet <at> ngyro.com> Cc: 51838 <at> debbugs.gnu.org, Pierre Langlois <pierre.langlois <at> gmx.com>, Jelle Licht <jlicht <at> fsfe.org> Subject: [bug#51838] [PATCH v5 07/45] guix: node-build-system: Add #:absent-dependencies argument. Date: Mon, 20 Dec 2021 14:33:34 -0500
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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.