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

[Jelle Licht]

Hey folks,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

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

I think we are drifting a bit off-topic here.

>>  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.
>
What are you arguing against/for? 

>> 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" [...]))
>

I'm guessing that's a retorical question, but I vastly prefer the second
over the first because it is parseable boilerplate:

- This means we could add a transformation to define package variants
- This also means we could programmatically rewrite some of this code
  later, `guix style'-style.

With the first approach you are 'stuck' with something that no sane
person will ever manually refactor for any significant number of
packages.

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

Too late for that I'm afraid :).

>> > 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 think this is an unfair assesment.

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

My poor T400 takes about 10 seconds to render the entire conversation,
indeed.

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

To be fair, you can always add a phase (and remove existing phases), so
this is a bit of a silly argument. Power of Guile Scheme and all that ;)

>> 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 think you are _really_ underestimating how many packages will need a
phase like this in the future. I would agree with this approach if it
were only needed incidentally, similar to the frequency of patching
version requirements in setup.py or requirements.txt for python
packages.

Pretty much everything except the '("node-tap") list will be identical
between packages; how do you propose we reduce this duplication? At this
point I feel like I'm rehasing the opposite of your last point, so let
me rephrase; how many times do you want to see/type/copy+paste the above
snippet before you would consider exposing this functionality on a
higher level?

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

This would be the equivalent to 'magically' patching out any dependency
that guix can't find. I like it in principle, but how is this
functionally different from the current approach of simply not checking
any node dependencies by deleting the configure phase? Perhaps I
misunderstood something somewhere along the way.

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

I believe the best thing to do would be to push the earlier
uncontroversial node patches.

Perhaps we can get some of the gurus/victims of other build systems
involved on guix-devel as none of the fundamental issues you've been
talking about for a while are node-specific. As long as we want to reach
some kind on consensus, I believe writing/reviewing more code does not
get us to a desirable outcome at this time.

- Jelle