[bug#51838] [PATCH v8 00/41] guix: node-build-system: Support compiling add-ons with node-gyp.

[Jelle Licht]

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

> Am Freitag, dem 07.01.2022 um 22:02 +0100 schrieb Jelle Licht:
>> To put it another way: a package that has to delete about 50
>> dependencies before it can be packaged in guix proper is allowed to
>> look ugly!  It can serve as a very unambigous list of things to
>> possibly improve in a package, while being easy to review as well if
>> changes are later proposed (e.g., you see 'node-karma-runner' being
>> added to inputs, as well as the "karma-runner" string being removed
>> from the list of dependencies to patch out).  With regex you always
>> need to hunt down if what is actually happening is what was intended
>> to happen.
>> 
>> In addition, since the things-that-can-be-matched by the regex w.r.t.
>> a particular package.json file are already known and clearly
>> enumerated, the only advantage regex brings us here is brevity. The
>> biggest advantage of not using regex is that after a later package
>> update, you can't inadvertently patch out a dependency that was added
>> by upstream.
> I'm not sure this is a good argument.  Even with a huge ass list (put
> the hyphen where you want to), upstreams can strengthen and weaken
> coupling with an update, both incidentally and purposefully.  So let's
> say we decided to delete node-karma for version 0.2.0 of a package and
> that worked fine until 0.6.5, but it's a requirement in 0.7.0 -- not
> that we'd run tests or anything, because those require tap -- I don't
> think either form is particularly helpful and in fact, I'd urge
> reviewers to take a close look at the package.json before and after
> regardless form.

Rather the other way around; if we do not allow regex, we will see that
there is a new dependency that we are currently neither patching nor
adding to the inputs. We should celebrate any accurate build time
failures we can make happen!

>> Before the DRY brigade comes to take my guix REPL, I'd argue that any
>> duplication here is incidental. What happens if we want to patch out
>> "karma" and "karma-chrome-launcher", but for some reason want to keep
>> "karma-browserify"?[1] Either way, the reviewer has to do a double
>> take to see whether a change from "karma.*" to the two specifications
>> "karma-chrome-launcher" and "karma" actually gives the expected
>> output w.r.t. the package.json file that is being patched.
> I think node packages do generally follow a pattern here, but let's
> assume they don't and your example works that way.  In that case, I'd
> argue to make that regexp "karma(-browserify)?".  And if later on
> karma-chrome-launcher is to be added to that list, but karma-icecat is
> allowed, "karma(-(browserify|chrome-launcher))?"  In other words, we
            ^
            I would object to this (constructed example) in a patch
review, fwiw.

At this stage, we expect the author to determine which dependencies to
leave in and out, then correctly encode these choices as a regex, which
is then decoded again by a reviewer who then must manually verify that
this decoded intent makes sense with regards to both the listed inputs
and the actual unpatched contents of package.json.

Without regex, there's just a few details that need to be kept in mind
for each step, for both author and reviewer(s).

Adding something to the deleted dependencies can either:
- fix a (newly) broken build
- remove an optional dependency, and can most likely be removed from
  existing inputs.

Removing something from the deleted dependencies can either:
- be a no-op: so the existing deleted dependency was (made) redundant
- enable an optional dependency, which should most likely be added to
  the inputs.

That's it! Some creative types can put this in fancy decision graph and
we have our "You Too Can Review Node Packages" campaign!

This all just to clarify my position, but perhaps also demonstrate that
it's a subjective preference, so 'agree to disagree' is a fine position
to take here.

> can as a community discourage the usage of ".*" without condemning all
> regexp.
>
> Now I'm personally not convinced that disallowing "karma.*" altogether
> is useful if we don't even have a karma package to go with -- and I'm
> very sure we'd notice karma being packaged and check our karma dropping
> packages -- but I'm willing to accept de gustibus here.

This is a very pragmatic example indeed! If some other folks could still
weigh in, I'd be okay with the resulting decision either way.

- Jelle