guix-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[bug#51838] [PATCH v8 00/41] guix: node-build-system: Support compiling


From: Jelle Licht
Subject: [bug#51838] [PATCH v8 00/41] guix: node-build-system: Support compiling add-ons with node-gyp.
Date: Fri, 07 Jan 2022 22:02:04 +0100

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

> Hi,
>
> Am Freitag, dem 07.01.2022 um 11:49 -0500 schrieb Timothy Sample:
>> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
>> > 4. Regexps :)
>> 
>> I doubt regex support will be broadly useful here.  Putting the
>> anchors in every package name (e.g., "^tap$") makes for a lot of
>> noise.  My (wild) guess would be that regexes will save us listing
>> two dependencies for one out of every ten Node packages.  Given that,
>> my preference would be to not bother with regex support.
> My reason to include them is that we can already see a number of
> packages requiring typescript(.*) for a number of (.*) -- similarly
> karma and mocha -- in the patch set given by Philip.  I do think
> regexps will be less useful later on and could very well become
> obsolete by the time we have a full bootstrap of everything, but we
> don't have an ETA on that, so for now I'd like to have that capability.
>
> I see two potential solutions here.  First is matching the whole string
> as you suggested and discussed below.  Second is opting in to regexp in
> general (there are some ".js" things, that would otherwise require
> escaping).  This would take the form of the user writing
> '("foo" "bar" (regexp "^baz$")) as UNWANTED, and it'd be interpreted as
> the predicates
> (equal? S "foo") ; alternatively string=?
> (equal? S "bar")
> (string-match S "^baz$")
> WDYT?

Or option three, which is the simplest one; no regex (regexen?), just a
big bag of boring strings. To be honest, the current set of packages
don't convince me of a need for such a facility yet. 

>> > 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.
>> 
>> If nothing else, I’m certainly on the other side of this debate!  :)
>> If every string is going to be treated as a pattern, we should have
>> it match fully by default.  That is, the anchors should be implicit. 
>> For the very rare (never?) case where you want to avoid anything that
>> so much as has “foo” in the name, it’s pretty easy to write
>> ".*foo.*".
> Full string matches are my preference too, but I only found the
> function that does partial match.  Is there an easier solution than
> checking whether the matched string equals the input to string-match?
>

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.

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 appreciate that this patch series is being dragged to the finish line
and give my gratitude to everyone involved.

 - Jelle

[1] I know this particular example is ridiculous, but the fact that you
have to know what these karma things are and how they relate to
eachother to be able to properly review this makes never getting into
such a situation all the more worth it to me.





reply via email to

[Prev in Thread] Current Thread [Next in Thread]