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

[Liliana Marie Prikler]

Hi,

Am Montag, dem 20.12.2021 um 14:33 -0500 schrieb Philip McGrath:
> 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?
> 
> [...]
> 
> 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.
Sure.  While we're at it, let's add k #:absent-dependencies-esque
fields, because surely the big O notation is an accurate measurement of
painful monkey work.

We are talking about human labour and Kolmogorov complexity here, both
of which I'd like to keep low, kthxbye. 

>  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.
This might be fine and dandy if you only have one or two packages which
actually need this crutch, but when you start summoning a demon from
the seventh layer of hell to make your particular pattern-obsessed
hello world program work, you will curse your past self for being so
stubborn and not implementing something that could leverage the
expressiveness of a programming language.

> 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.
How are upstream sources not a source of truth here?  If anything, you
would have to always check that whatever hack you employed back then
still produces a functional package and #:absent-dependencies is not
helping in that.

> 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.
Whoever contributes node-tap will not be responsible to update any
package that might want to take advantage of it.  They can go out of
their way to add it, but the idea, that "I have to update 300 packages
in each and every patch set" is flawed from the get-go.  We can rely on
the community's collective brain to either remember in the future that
node-tap was optional for some package and add it or to not care. 
Whether they are aided by comments or glorified comments (or not) makes
little difference at that point.

> > 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. 
At least within GNU build system there's the convention of passing
TESTS="subset of tests you want" to your invocation of make check.  The
meson code could also be adapted to such a use-case.  It still doesn't
make sense to do so.

> 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. 
Guess what, even with #:configure-flags, we have to replace the
configure phase to *only* use #:configure-flags in certain packages. 
Then again, if node supported --without-left-pad, we wouldn't be here
discussing #:absent-dependencies, would we?

> 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.
"Readable" is quite a stretch here.  I prefer "parseable boilerplate".
What's more readable?

  (#:strict? #f) ; node-tap... 
  (#:absent-dependencies '("node-tap" "node-tap-the-cloud" "node-tap-
more" "node-tap-pat" "node-tap-atapter" "node-tap-left-pad" [...]))

> 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.)
Both statements are correct.  My first suggestion was indeed to just
issue a warning, but after thinking about it harder, I feel as though
it shouldn't even be the responsibility of the patch-dependencies phase
to act as a generic json rewriting tool.  The `patch-dependencies'
phase is simply named that because we're not Java and thereby have no
moral obligation to name it
patchDependenciesAndWhileYoureAtItAlsoInlineDevDependenciesIntoTheMainT
hingKThxBye.

Back then, I thought that rewriting the package.json to reflect the
inputs during build ought to be the simple and correct choice, but I do
agree that at certain times you might also be right and deleting a
single dependency from the tree is the correct option.  So I think we
have a problem for which there cannot be a high-level solution in the
build system and we need to write #:phases into the packages, not the
build system.  The build system does need to provide some primitives to
make that simple, though, e.g. with-atomic-json-replacement.

> > > 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.
Merriam-Webster defines gratuitous as "don't fucking quote Merriam-
Webster during code review".
Your patch might itself not break anything, but the patch to remove it
and update it with something better will.  And as an armchair software
architect, I sit firmly in the "do it right the first time" camp.

> > 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.
If this is out-of-scope for the series, then so is #:absent-
dependencies.  Please rewrite your series to not require a keyword
addition then and have fun building your new packages with substitute*.

I'm really not trying to be mean here in holding back your patch
without good reason, but I do think that there's enough things to fix
to require a v6 (perhaps even a v7, but let's stay optimistic) before
we can upstream this in good conscience.  There's also the fact, that
(as Jelle pointed out) we've discussed more than we've written patches.
If I wanted to dictate a solution here, I could easily have submitted a
v6 on my own at some time during review, but in my personal opinion
that doesn't help much in reaching a consensus.

> > 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.
#:absent-dependencies is brittle boilerplate and at the same time
extremely limited.
My initially suggested "warn, not fail" is somewhat less limited and
not boilerplate, but still brittle in another way (giving gratuitous
runtime errors).
Adding a phase opens up all the power of Guile Scheme, making the
package exactly as sensitive to errors as you want it to be, plus it
requires only minimal change in the API in the form of more exported
functions, but no changed calling conventions.

There ought to be no question as to which option is the superior one
here :)

> 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")
> ```
That is the point, but please don't add a function called "make-delete-
dependencies-phase".  We have lambda.  We can easily add with-atomic-
json-replacement.  We can also add a "delete-dependencies" function
that takes a json and a list of dependencies if you so want.

So in short

(add-after 'patch-dependencies 'drop-junk
  (lambda _
    (with-atomic-json-replacement "package.json"
      (lambda (json) (delete-dependencies json '("node-tap"))))))

would be the "verbose" style of disabling a list of dependencies.

> 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.
If you want something that's not verbose and declarative, implement
#:strict? #f.  #:absent-dependencies is extremely verbose for what it
achieves, particularly when we think about the implementation as well.

> 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?
To be clear, I never demanded you fix all the bad code in node-build-
system or something like that.  I only pointed out issues, which are
adjacent to the patch set at hand and more importantly those that we
could "easily" fix with tools that we already have at our disposal. 
Perhaps I am misjudging the difficulty of some tasks involved here, but
I haven't really seen a call for help in your replies.  If you do think
I'm pushing unfair amounts of work onto you, please say so.  If not,
then happy hacking :)