[bug#51838] [PATCH v6 05/41] guix: node-build-system: Add 'delete-dependencies' helper function.

[Liliana Marie Prikler]

Hi,

Am Donnerstag, dem 30.12.2021 um 20:09 -0500 schrieb Philip McGrath:
> Hi,
> 
> On 12/30/21 12:29, Liliana Marie Prikler wrote:
> > Am Donnerstag, dem 30.12.2021 um 02:38 -0500 schrieb Philip
> > McGrath:
> > > +  (define delete-fom-jsobject
For the record, I forgot to mention the typo here.  It obviously ought
to be delete-from-jsobject.

> [...]
> I don't think '#:json-keys' would be helpful.
> 
> In my view, the high-level purpose of 'delete-dependencies', 
> '#:absent-dependencies', or whatever is to gather our collective 
> procedural knowledge about how to modify a "package.json" file to
> build a package without some of the dependencies its developers have
> declared and to encode that knowledge in a single, abstracted point
> of control in 'node-build-system', so that authors of Node.js package
> definitions can simply specify which declared dependencies are absent
> and leave it to 'node-build-system' to act accordingly. (I don't
> think it matters _why_ the dependencies are absent, i.e. whether we
> don't want the them or merely don't have them.)
For the record, you can delete present dependencies as well, which is
one shortcoming in the "absent dependencies" metaphor.  As for deleting
absent packages automatically without specifying them, one could argue
that a shortcoming of this set is that it doesn't provide a way of
doing that.  I'll take a closer look at the ease-of-use as I walk down
your reply.

> In our experience so far, the necessary modification does concretely 
> amount to "Remove DEPENDENCIES from JSON_KEYS in FILE.", but that is
> not the ultimate purpose of this code, and I think that description,
> along with '#:json-keys', ends up being simultaneously too flexible
> and too restrictive. 
Admittedly, if you read DEPENDENCIES, JSON_KEYS and FILE as mere
variables with no meaning that's bad, but the key here is that
DEPENDENCIES is a list of dependencies and JSON_KEYS is a list of json
keys that refer to dependency lists.  Anything else would be unlawful
use.

> It is unnecessarily flexible because, if a package author ever passes
> some other value for '#:json-keys', that would seem to indicate that
> there's some procedural knowledge missing from 'node-build-system',
> and it should be added there.
The reason I have it as such is that a packager might decide in the
future to e.g. only drop a peer dependency, that might share a name
with a dev dependency or something along those lines.  Along with
#:file, it's also supposed to guard against files other than
package.json containing a list of dependencies that blows up in our
faces, which we could edit with the same primitive.  That being said, I
do get your point in that it's overly flexible for now in which the
point is just to rewrite package.json.

> More significantly, it unnecessarily seems to restrict 'delete-
> dependencies' from taking other kinds of actions to handle the absent
> dependencies, if in the future we should discover that there's
> something we need to do that wouldn't amount to just adding another
> JSON key. It's a little odd to give an example of something we might
> not know, but, for example, I could imagine learning that correct
> handling of absent "peerDependencies" could require more involved
> transformation of the structures under "peerDependenciesMeta".
Do you mean that as in "if a dependency is found under this key, then
foo, bar and baz also need to happen"?  If so, that would be
concerning, but also a good reason to have that abstraction.

> As far as the rest of your suggestion, on the one hand, this:
> 
>    (define* (delete-dependencies deps #:key (file "package.json"))
>      (with-atomic-json-file-replacement ...))
> 
> seems like a fine enhancement, and I could live with it---I'd even 
> prefer it, if v6 but not v7 of this patch series can achieve
> consensus.
Given that you deleted keys, I don't think there's a good reason to
keep around file... or can it be located in a subdirectory?

> On the other hand, at the risk of beating a dead horse, it seems like
> a tiny step from the above to:
> 
>    (define* ((delete-dependencies deps #:key (file "package.json")) .
> _)
>      (with-atomic-json-file-replacement ...))
> 
> which is just another name for 'make-delete-dependencies-phase',
> which AIUI you had found objectionable. (Apparently that shorthand
> would need (ice-9 curried-definitions).)
The reason to not use a phase writer here, is that you could combine it
with other stuff in a single phase.  E.g.

  (add-after 'unpack 'remove-foo
    (lambda _
      (delete-dependencies '("foo" "bar" "baz"))
      (delete-file "baz-loader.js")))

> Indeed, if we observe that '#:file', similarly to '#:json-keys', will
> never be anything _other_ than "package.json", we could further
> simplify to:
> 
>    (define* ((delete-dependencies deps) . _)
>      (with-atomic-json-file-replacement ...))
> 
> at which point we've basically re-invented the implementation of
> patch v7 05/41, which basically amounts to:
> 
>    (define* (delete-dependencies #:key absent-dependencies)
>      (with-atomic-json-file-replacement ...))
> 
> In other words, I don't agree that any of these possible changes
> would "eliminat[e] the need to shorten it to #:absent-dependencies",
Sorry for the typo.

> I still feel that there's something I'm fundamentally not
> understanding about your objections to '#:make-absent-dependencies',
> which is why, in v6, I tried to do exactly as you had proposed:
> 
> On 12/20/21 17:00, Liliana Marie Prikler wrote:
>  > Hi Timothy,
>  >
>  > Am Montag, dem 20.12.2021 um 15:15 -0500 schrieb Timothy Sample:
>  >> Hi Philip,
>  >>
>  >> Philip McGrath <philip@philipmcgrath.com> writes:
>  >>
>  >>> 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"))))
>  >>> ```
>  >>
>  >> I’m perfectly happy with this if it’s a compromise we all can
> agree on.
>  >> It is exactly what popped into my imagination when I read
> Liliana’s
>  >> suggestion.  I guess the one thing missing is that it would not
>  >> necessarily be implemented on top of better “package.json”
>  >> manipulation support.  That said, it doesn’t preclude providing
> that
>  >> support if/when the need arises.
>  > In my personal opinion, we would write that support first and
> perhaps
>  > the shorthands later.  I.e.
>  >
>  > (add-after 'patch-dependencies 'drop-junk
>  >    (lambda _
>  >      (with-atomic-json-replacement "package.json"
>  >        (lambda (json) (delete-dependencies json '("node-tap"))))))
To be fair, finding the right sweet spot between being overly verbose
and code golfing is difficult.

> Certainly I do agree that it would be better to support code more 
> concise than that! But I think all of these variations are strictly 
> worse than '#:absent-dependencies'. It isn't just that they are more 
> typing: the require authors of package definitions to have some (not 
> very much, but some) procedural knowledge about _how_ 'node-build-
> system' deals with the absence of dependencies, rather
> than with '#:absent-dependencies', declaratively specifying _what_ is
> to be done. 
Understanding build systems is for nerds.  We here at leftpad.org care
about the things that are really important.

> For example, as I mentioned in my cover letter at 
> <https://issues.guix.gnu.org/51838#257>, even my own code from the 
> exchange I just quoted:
> 
>  >>>    (add-after 'unpack 'delete-dependencies
>  >>>      (make-delete-dependencies-phase '("node-tap"))))
> 
> would be broken in v6, because the implementation of 
> 'delete-dependencies' assumes that the 'patch-dependencies' phase has
> already been run. 
I think that is an issue with your patch, however.  With mine, you
could at least add "peerDependencies" on your own.

> I think this is an implementation detail that users of 
> 'node-build-system' should not be required to know! Indeed, I think
> that would be a good reason to have 'patch-dependencies' handle the
> absent dependencies itself [...]
I think something like that can be arranged.  We could make patch-
dependencies drop all the dependencies it doesn't know about if we add
a "verify-dependencies" phase before it.  (Note that I already
suggested that once).  verify-dependencies would raise an error if a
dependency is missing and the user could then decide to drop it or
(add-before 'verify-dependencies 'drop-dependencies ...).

> I expect the majority of Guix's Node.js packages will continue for
> the foreseeable future to need the functionality for absent
> dependencies I described at the beginning of this email, so I think
> we should provide it through a mechanism that is as high-level,
> concise, declarative as possible, and ideally one that will
> facilitate automated code generation and static analysis. On each of
> these criteria, I think '#:absent-dependencies' is better than any of
> the other proposals I've heard.
> 
> But, as I said, in the interest of compromise and moving forward, I'm
> willing to live with something based on v6 for now if that's what can
> achieve consensus, and then propose '#:absent-dependencies'
> separately.  So, if you want me to send a new version with one of
> these other variations, tell me which one, and I'll do it.
I think amending v6 with what we learn from this discussion and the
discussion on patch 3 of this series is the way to go.

Another thing I forget to mention all the time are regexps.  I think
it'd be beneficial if delete-dependencies could delete dependencies
based on their name matching a regexp rather than a string exactly. 
This would make some of your lists shorter (e.g. "karma.*"), but there
might be a debate on whether to use "^karma.*$" or whether to only
consider regexps that match the dependency fully.

> I hope my tone isn't coming across the wrong way---I really don't
> mean to be snarky! But I am genuinely struggling to understand the 
> significance of the difference between:
> 
>  >>>    (add-after 'unpack 'delete-dependencies
>  >>>      (make-delete-dependencies-phase '("node-tap"))))
> 
> which I thought you objected to, and the result of what I think
> you've most recently proposed:
> 
>      (add-after 'patch-dependencies 'delete-dependencies
>        (lambda () (delete-dependencies '("node-tap"))))
To be clear, there are two things in here which I objected.
1. make-delete-dependencies-phase as a procedure which returns the
actual phase instead of writing the lambda out.  BTW if I ever wrote
(lambda () (delete-dependencies ...)), that was a mistake on my part.
As for the reason, scroll up.
2. implementing delete-dependencies in terms of messy ad-hoc json
primitives using inner defines rather than reusable ones.

The goal was not to create the most unwieldy incantation even though it
certainly appears as though it was looking back.  Sorry for the
misunderstanding.

> which would have avoided my earlier reservations about making the
> JSON representation part of the build system's public API for the
> first time.
> 
> So I'm not feeling very confident in my ability to predict what
> changes would or would not block consensus.
Adding a gratuitous keyword is an immediate blocker as we've discussed
at length ;)
Using curried functions is also unlikely going to meaningfully advance
the discussion, but there could be a niche application in which they're
actually useful that I had not thought about.

Apart from that, there is a large room simply for discussions, where it
is unclear what form consensus will take before it is shaped.  For
instance, for patch 3, you had to write a lot of functionality from
scratch, which prompted me to suggest that you put them in their own
file (which would then be public API, though), but with a little bit of
refactoring in terms of SRFI-1 we might make it small enough to keep to
npm-build-system until Rust folks demand the same goodies.  Phase
ordering as you mentioned is also still up to debate.  So there's a lot
in which you can can add your opinion.

If you are doubtful about whether or not a particular change would be
good, you could probe different versions you have in your head and ask
for opinions on them.  E.g. provide two or three implementations of
assoc-set and let the "best" be selected.  If you only have a single
one, you can still ask if something like that would be okay and if you
receive a "No" that you don't understand it's okay to ask "what would
be the problem, what solution do you propose?" etc.  XKCD's "inventing
a new standard to cover both use-cases" applies as always too :)

Cheers