| Message ID | 20211217020325.520821-7-philip@philipmcgrath.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | guix: node-build-system: Support compiling add-ons with node-gyp. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
Hi, Am Donnerstag, dem 16.12.2021 um 21:02 -0500 schrieb Philip McGrath: > * guix/build/node-build-system.scm (patch-dependencies): Strictly > follow the linearity rules for `assoc-set!` and friends. > Clarify the types of the arguments to and return value from the > internal helper function `resolve-dependencies`. > [...] > - (define (resolve-dependencies package-meta meta-key) > - (fold (lambda (key+value acc) > - (match key+value > - ('@ acc) > - ((key . value) (acons key (hash-ref index key value) > acc)))) > - '() > - (or (assoc-ref package-meta meta-key) '()))) > + (define (resolve-dependencies meta-alist meta-key) > + ;; Given: > + ;; - The alist from "package.json", with the '@ unwrapped > + ;; - A string key, like "dependencies" > + ;; Returns: an alist (without a wrapping '@) like the entry in > + ;; meta-alist for meta-key, but with dependencies supplied > + ;; by Guix packages mapped to the absolute store paths to use. > + (match (assoc-ref meta-alist meta-key) > + (#f > + '()) > + (('@ . orig-deps) > + (fold (match-lambda* > + (((key . value) acc) > + (acons key (hash-ref index key value) acc))) > + '() > + orig-deps)))) > > (with-atomic-file-replacement "package.json" > (lambda (in out) > - (let ((package-meta (read-json in))) > - (assoc-set! package-meta "dependencies" > - (append > - '(@) > - (resolve-dependencies package-meta > "dependencies") > - (resolve-dependencies package-meta > "peerDependencies"))) > - (assoc-set! package-meta "devDependencies" > - (append > - '(@) > - (resolve-dependencies package-meta > "devDependencies"))) > + ;; It is unsafe to rely on 'assoc-set!' to update an > + ;; existing assosciation list variable: > + ;; see 'info "(guile)Adding or Setting Alist Entries"'. > + (let* ((package-meta (read-json in)) > + (alist (match package-meta > + ((@ . alist) alist))) > + (alist > + (assoc-set! > + alist "dependencies" > + (append > + '(@) > + (resolve-dependencies alist "dependencies") > + (resolve-dependencies alist "peerDependencies")))) > + (alist > + (assoc-set! > + alist "devDependencies" > + (append > + '(@) > + (resolve-dependencies alist "devDependencies")))) > + (package-meta (cons '@ alist))) > (write-json package-meta out)))) > #t) The Guix codebase is generally not the place to play around with destructive semantics. If you can avoid assoc-set!, I think you ought to, especially if it helps making a two-step process into a single-step one. Anything I'm missing here?
Hi, On 12/16/21 23:29, Liliana Marie Prikler wrote: > Hi, > > Am Donnerstag, dem 16.12.2021 um 21:02 -0500 schrieb Philip McGrath: >> * guix/build/node-build-system.scm (patch-dependencies): Strictly >> follow the linearity rules for `assoc-set!` and friends. >> Clarify the types of the arguments to and return value from the >> internal helper function `resolve-dependencies`. >> [...] >> - (define (resolve-dependencies package-meta meta-key) >> - (fold (lambda (key+value acc) >> - (match key+value >> - ('@ acc) >> - ((key . value) (acons key (hash-ref index key value) >> acc)))) >> - '() >> - (or (assoc-ref package-meta meta-key) '()))) >> + (define (resolve-dependencies meta-alist meta-key) >> + ;; Given: >> + ;; - The alist from "package.json", with the '@ unwrapped >> + ;; - A string key, like "dependencies" >> + ;; Returns: an alist (without a wrapping '@) like the entry in >> + ;; meta-alist for meta-key, but with dependencies supplied >> + ;; by Guix packages mapped to the absolute store paths to use. >> + (match (assoc-ref meta-alist meta-key) >> + (#f >> + '()) >> + (('@ . orig-deps) >> + (fold (match-lambda* >> + (((key . value) acc) >> + (acons key (hash-ref index key value) acc))) >> + '() >> + orig-deps)))) >> >> (with-atomic-file-replacement "package.json" >> (lambda (in out) >> - (let ((package-meta (read-json in))) >> - (assoc-set! package-meta "dependencies" >> - (append >> - '(@) >> - (resolve-dependencies package-meta >> "dependencies") >> - (resolve-dependencies package-meta >> "peerDependencies"))) >> - (assoc-set! package-meta "devDependencies" >> - (append >> - '(@) >> - (resolve-dependencies package-meta >> "devDependencies"))) >> + ;; It is unsafe to rely on 'assoc-set!' to update an >> + ;; existing assosciation list variable: >> + ;; see 'info "(guile)Adding or Setting Alist Entries"'. >> + (let* ((package-meta (read-json in)) >> + (alist (match package-meta >> + ((@ . alist) alist))) >> + (alist >> + (assoc-set! >> + alist "dependencies" >> + (append >> + '(@) >> + (resolve-dependencies alist "dependencies") >> + (resolve-dependencies alist "peerDependencies")))) >> + (alist >> + (assoc-set! >> + alist "devDependencies" >> + (append >> + '(@) >> + (resolve-dependencies alist "devDependencies")))) >> + (package-meta (cons '@ alist))) >> (write-json package-meta out)))) >> #t) > The Guix codebase is generally not the place to play around with > destructive semantics. If you can avoid assoc-set!, I think you ought > to, especially if it helps making a two-step process into a single-step > one. Anything I'm missing here? I agree that assoc-set! is best avoided. (I am a Racketeer: we don't mutate pairs.) However, this code was already using assoc-set!: the change in this patch is merely to use it correctly. AFAIK neither `info "(guile)Association Lists"` nor SRFI-1 (`info "(guile)SRFI-1 Association Lists"`) provide a non-destructive assoc-set operation. Note in particular that acons and alist-cons do not work, since they don't remove existing entries for the same key: they would result in duplicate keys being written to the JSON object. (In theory, this has undefined semantics; in practice, I believe Node.js would use the wrong entry.) Of course, I know how to write a little library of purely functional association list operations---but that seems vastly out of scope for this patch series (which has already grown quite large). Furthermore, if we were going to make such changes, I think it might be better to change the representation of JSON objects to use a real immutable dictionary type, probably VHash (though it looks like those would still need missing functions, at which point a wrapper type that validated keys and maintained a consistent hash-proc might be even better). Alternatively we could use guile-json, which at least avoids the need for improper alists to disambiguate objects from arrays, but we would have to address the issues in Guix commit a4bb18921099b2ec8c1699e08a73ca0fa78d0486. All of that reinforces my sense that we should not try to change this here. -Philip
Hi, Am Samstag, dem 18.12.2021 um 12:03 -0500 schrieb Philip McGrath: > [...] > > > The Guix codebase is generally not the place to play around with > > destructive semantics. If you can avoid assoc-set!, I think you > > ought to, especially if it helps making a two-step process into a > > single-step one. Anything I'm missing here? > > I agree that assoc-set! is best avoided. (I am a Racketeer: we don't > mutate pairs.) However, this code was already using assoc-set!: the > change in this patch is merely to use it correctly. > > AFAIK neither `info "(guile)Association Lists"` nor SRFI-1 (`info > "(guile)SRFI-1 Association Lists"`) provide a non-destructive assoc- > set operation. Note in particular that acons and alist-cons do not > work, since they don't remove existing entries for the same key: they > would result in duplicate keys being written to the JSON object. (In > theory this has undefined semantics; in practice, I believe Node.js > would use the wrong entry.) > > Of course, I know how to write a little library of purely functional > association list operations---but that seems vastly out of scope for > this patch series (which has already grown quite large). Furthermore, > if we were going to make such changes, I think it might be better to > change the representation of JSON objects to use a real immutable > dictionary type, probably VHash (though it looks like those would > still need missing functions, at which point a wrapper type that > validated keys and maintained a consistent hash-proc might be even > better). Alternatively we could use guile-json, which at least avoids > the need for improper alists to disambiguate objects from arrays, but > we would have to address the issues in Guix commit > a4bb18921099b2ec8c1699e08a73ca0fa78d0486. > > All of that reinforces my sense that we should not try to change this > here. I think you misread me here. One thing that's bugging me is that you (just like whoever wrote this before) strip the @ only to reintroduce it. I think it'd be better if (resolve-dependencies) simply took a list and the let-block deconstructed the json. As for the package-meta -> package-meta conversion, imo that could perfectly be done with match or SXML transformation. WDYT?
Hi Liliana, Liliana Marie Prikler <liliana.prikler@gmail.com> writes: > Am Samstag, dem 18.12.2021 um 12:03 -0500 schrieb Philip McGrath: > >> All of that reinforces my sense that we should not try to change this >> here. > > I think you misread me here. One thing that's bugging me is that you > (just like whoever wrote this before) strip the @ only to reintroduce > it. I think it'd be better if (resolve-dependencies) simply took a > list and the let-block deconstructed the json. > > As for the package-meta -> package-meta conversion, imo that could > perfectly be done with match or SXML transformation. WDYT? I think that, as a patch, this is a clear improvement over the existing code. Is it perfect yet? Maybe not. Further improvements can always be made. The NPM package set is small enough that build system changes can be committed directly to master. Each of the three of us has spent more time writing about it than it would take to reimplement it! :) To me, it would be unwise to hold back this series because one of the patches replaces ugly and broken mutations with merely ugly mutations. -- Tim
Hi, On 12/18/21 12:52, Liliana Marie Prikler wrote: > Hi, > > Am Samstag, dem 18.12.2021 um 12:03 -0500 schrieb Philip McGrath: >> [...] >> >>> The Guix codebase is generally not the place to play around with >>> destructive semantics. If you can avoid assoc-set!, I think you >>> ought to, especially if it helps making a two-step process into a >>> single-step one. Anything I'm missing here? >> >> I agree that assoc-set! is best avoided. (I am a Racketeer: we don't >> mutate pairs.) However, this code was already using assoc-set!: the >> change in this patch is merely to use it correctly. >> >> AFAIK neither `info "(guile)Association Lists"` nor SRFI-1 (`info >> "(guile)SRFI-1 Association Lists"`) provide a non-destructive assoc- >> set operation. Note in particular that acons and alist-cons do not >> work, since they don't remove existing entries for the same key: they >> would result in duplicate keys being written to the JSON object. (In >> theory this has undefined semantics; in practice, I believe Node.js >> would use the wrong entry.) >> >> Of course, I know how to write a little library of purely functional >> association list operations---but that seems vastly out of scope for >> this patch series (which has already grown quite large). Furthermore, >> if we were going to make such changes, I think it might be better to >> change the representation of JSON objects to use a real immutable >> dictionary type, probably VHash (though it looks like those would >> still need missing functions, at which point a wrapper type that >> validated keys and maintained a consistent hash-proc might be even >> better). Alternatively we could use guile-json, which at least avoids >> the need for improper alists to disambiguate objects from arrays, but >> we would have to address the issues in Guix commit >> a4bb18921099b2ec8c1699e08a73ca0fa78d0486. >> >> All of that reinforces my sense that we should not try to change this >> here. > I think you misread me here. One thing that's bugging me is that you > (just like whoever wrote this before) strip the @ only to reintroduce > it. I think it'd be better if (resolve-dependencies) simply took a > list and the let-block deconstructed the json. > > As for the package-meta -> package-meta conversion, imo that could > perfectly be done with match or SXML transformation. WDYT? > I definitely am not understanding what you have in mind here. When you write "strip the @", I'm not sure what you're referring to, because there are multiple "@" tags here, one beginning each JSON object. (Maybe this is obvious, but it hadn't been obvious to me.) So, the (guix build json) representation of a "package.json" file might look like this: ``` `(@ ("name" . "apple") ("version" . "1.0") ("dependencies". (@ ("banana" . "*") ("pear" . "*"))) ("devDependencies" . (@ ("peach" . "*") ("orange" . "*"))) ("peerDependencies" . (@ ("node-gyp" . "7.x"))) ("peerDependenciesMeta" . (@ ("node-gyp" . (@ ("optional" . #t))))) ("optionalDependencies" . (@ ("node-gyp" . "7.x")))) ``` An unfortunate consequence of this representation is that JSON objects are not usable directly as association lists: some procedures expecting association lists seem to silently ignore the non-pair at the head of the list, but I don't think that's guaranteed, and other procedures just don't work. In particular, `append` applied to two JSON objects does not produce a JSON object, even ignoring the problem of duplicate keys. Given that the current code adds "peerDependencies" as additional "dependencies", the choice (as I see it) is between the current approach, in which `resolve-dependencies` returns genuine association lists and the `let*` block turns them JSON objects, or changing `resolve-dependencies` to return JSON objects and implementing `json-object-append`, which doesn't seem obviously better, unless it were part of broader changes. (As an aside, I am not convinced that the current handling of "peerDependencies" is right, but I think reevaluating that behavior is out of scope for this patch series, and particularly for this patch, in which, as Tim said in <https://issues.guix.gnu.org/51838#234>, my goal was merely to make the use of `assoc-set!` safe.) I definitely think the broader situation should be improved! But I think those improvements are out of scope for this patch series. It seems like much more discussion would be needed on what the improvements should be, and potentially coordination with other users of (guix build json). Personally, I'd want to represent JSON objects with a real immutable dictionary type that gave us more guarantees about correctness by construction. If we continue with tagged association lists, we should write a little library of purely functional operations on JSON objects. But that all seems very far afield. -Philip
Hi, Am Montag, dem 20.12.2021 um 13:03 -0500 schrieb Philip McGrath: > I definitely am not understanding what you have in mind here. When > you write "strip the @", I'm not sure what you're referring to, > because there are multiple "@" tags here, one beginning each JSON > object. (Maybe this is obvious, but it hadn't been obvious to me.) I'm referring to this part: > + (define (resolve-dependencies meta-alist meta-key) > + ;; Given: > + ;; - The alist from "package.json", with the '@ unwrapped > + ;; - A string key, like "dependencies" > + ;; Returns: an alist (without a wrapping '@) like the entry in > + ;; meta-alist for meta-key, but with dependencies supplied > + ;; by Guix packages mapped to the absolute store paths to use. > + (match (assoc-ref meta-alist meta-key) > + (#f > + '()) > + (('@ . orig-deps) > + (fold (match-lambda* > + (((key . value) acc) > + (acons key (hash-ref index key value) acc))) > + '() > + orig-deps)))) You could simply write the function s.t. (resolve-dependencies DEPS) takes an alist and then produces an alist of resolved dependencies. Because you don't, you need to code around that quirk down in the file replacements. You can safely access the dependencies using (or (and=> (assoc "dependencies" json) cddr) '()) in the calling code. If you replace "dependencies" by a generic KEY, you can also outline that into a helper function. > So, the (guix build json) representation of a "package.json" file > might look like this: > > ``` > `(@ ("name" . "apple") > ("version" . "1.0") > ("dependencies". (@ ("banana" . "*") > ("pear" . "*"))) > ("devDependencies" . (@ ("peach" . "*") > ("orange" . "*"))) > ("peerDependencies" . (@ ("node-gyp" . "7.x"))) > ("peerDependenciesMeta" . (@ ("node-gyp" . (@ ("optional" . > #t))))) > ("optionalDependencies" . (@ ("node-gyp" . "7.x")))) > ``` Note that '("dependencies" . (@ ("banana" . "long") ("pear" . "*"))) is equal to '("dependencies" @ ("banana" . "long") ("pear" . "juicy")). > An unfortunate consequence of this representation is that JSON > objects are not usable directly as association lists: some procedures > expecting association lists seem to silently ignore the non-pair at > the head of the list, but I don't think that's guaranteed, and other > procedures just don't work. There are sloppy variants of the assoc functions, but again, you are looking at this from the wrong angle. Just ignore that you have a JSON object and pass the alist to resolve-dependencies, then reconstruct a JSON object from the result. ezpz > In particular, `append` applied to two JSON objects does not produce > a JSON object, even ignoring the problem of duplicate keys. I think we can ignore that for now, but if it bugs you you can convert to hash table and back or at least assert that the keys are unique. > Given that the current code adds "peerDependencies" as additional > "dependencies", the choice (as I see it) is between the current > approach, in which `resolve-dependencies` returns genuine association > lists and the `let*` block turns them JSON objects, or changing > `resolve-dependencies` to return JSON objects and implementing > `json-object-append`, which doesn't seem obviously better, unless it > were part of broader changes. The return value of resolve-dependencies is okay imo. It's the calling convention that is not. > (As an aside, I am not convinced that the current handling of > "peerDependencies" is right, but I think reevaluating that behavior > is out of scope for this patch series, and particularly for this > patch, in which, as Tim said in > <https://issues.guix.gnu.org/51838#234>, my goal was merely to make > the use of `assoc-set!` safe.) I agree that we can ignore the semantics of peerDependencies for now and we may even be able to look aside the use of assoc-set! (although for the purpose of making it easier for the user to rewrite those files on their own as I laid out before, it might still make sense to drop that use regardless, leading by example). > But I think those improvements are out of scope for this patch > series. I don't. Particularly, the current implementation quirks appear to be the sole reason you came up with the solution of #:absent-dependencies, which for the record I still disagree with. We can lay down a better foundation to rewrite JSON data in node-build-system and should export functions that are necessary to do so in build-side code if they're not already part of core guile or other imports. > It seems like much more discussion would be needed on what the > improvements should be, and potentially coordination with other users > of (guix build json). Personally, I'd want to represent JSON objects > with a real immutable dictionary type that gave us more guarantees > about correctness by construction. If we continue with tagged > association lists, we should write a little library of purely > functional operations on JSON objects. But that all seems very far > afield. I'll have to say non sequitur to that. The functionality we require to efficiently rewrite JSON can perfectly be built on top of (a)list primitives and pattern matching, both of which are available to be used in build-side code. We could even throw in SXML if we needed, not that we do. There is really no need to code up yet another set of JSON primitives just to write "hello, world". If you absolutely require it, though: (define json-object->alist cdr) (define (alist->json-object alist) (cons '@ alist)) Magic. Cheers
Hi Liliana, On 12/20/21 14:54, Liliana Marie Prikler wrote: > Hi, > > Am Montag, dem 20.12.2021 um 13:03 -0500 schrieb Philip McGrath: >> I definitely am not understanding what you have in mind here. When >> you write "strip the @", I'm not sure what you're referring to, >> because there are multiple "@" tags here, one beginning each JSON >> object. (Maybe this is obvious, but it hadn't been obvious to me.) > I'm referring to this part: >> + (define (resolve-dependencies meta-alist meta-key) >> + ;; Given: >> + ;; - The alist from "package.json", with the '@ unwrapped >> + ;; - A string key, like "dependencies" >> + ;; Returns: an alist (without a wrapping '@) like the entry in >> + ;; meta-alist for meta-key, but with dependencies supplied >> + ;; by Guix packages mapped to the absolute store paths to use. >> + (match (assoc-ref meta-alist meta-key) >> + (#f >> + '()) >> + (('@ . orig-deps) >> + (fold (match-lambda* >> + (((key . value) acc) >> + (acons key (hash-ref index key value) acc))) >> + '() >> + orig-deps)))) > You could simply write the function s.t. (resolve-dependencies DEPS) > takes an alist and then produces an alist of resolved dependencies. > Because you don't, you need to code around that quirk down in the file > replacements. You can safely access the dependencies using > (or (and=> (assoc "dependencies" json) cddr) '()) > in the calling code. If you replace "dependencies" by a generic KEY, > you can also outline that into a helper function. > >> An unfortunate consequence of this representation is that JSON >> objects are not usable directly as association lists: some procedures >> expecting association lists seem to silently ignore the non-pair at >> the head of the list, but I don't think that's guaranteed, and other >> procedures just don't work. > There are sloppy variants of the assoc functions, but again, you are > looking at this from the wrong angle. Just ignore that you have a JSON > object and pass the alist to resolve-dependencies, then reconstruct a > JSON object from the result. ezpz To check my understanding, are you saying you'd like the code to look like this? (n.b. not tested) ``` (define* (patch-dependencies #:key inputs #:allow-other-keys) (define index (index-modules (map cdr inputs))) (define (resolve-dependencies orig-deps) (fold (match-lambda* (((key . value) acc) (acons key (hash-ref index key value) acc))) '() orig-deps)) (define (lookup-deps-alist meta-alist meta-key) (match (assoc-ref meta-alist meta-key) (#f '()) (('@ . deps) deps))) (with-atomic-file-replacement "package.json" (lambda (in out) (let* ((package-meta (read-json in)) (alist (match package-meta ((@ . alist) alist))) (alist (assoc-set! alist "dependencies" (cons '@ (resolve-dependencies (append (lookup-deps-alist alist "dependencies") (lookup-deps-alist alist "peerDependencies")))))) (alist (assoc-set! alist "devDependencies" (cons '@ (resolve-dependencies (lookup-deps-alist alist "devDependencies"))))) (package-meta (cons '@ alist))) (write-json package-meta out)))) #t) ``` I wouldn't have messed with it if I'd found it this way, but to me it does not seem obviously better. In particular, I think any benefit of simplifying `resolve-dependencies` is outweighed by `lookup-deps-alist` conflating looking up the key in the given alist with unwrapping the result, if there is one. Just to be explicit, your much more elegant code with `match-lambda` from above: > (map (match-lambda > (('dependencies '@ . DEPENDENCIES) > (filter away unwanted dependencies)) > (('devDependencies '@ . DEPENDENCIES) > (same)) > (otherwise otherwise)) > json))))) wouldn't quite be enough here: it's possible for the "dependencies" key to be absent but the "peerDependencies" key to be present, in which case, to preserve the current behavior, we still need to add the filtered/rewritten "peerDependencies" as "dependencies". >> But I think those improvements are out of scope for this patch >> series. > I don't. Particularly, the current implementation quirks appear to be > the sole reason you came up with the solution of #:absent-dependencies, > which for the record I still disagree with. We can lay down a better > foundation to rewrite JSON data in node-build-system and should export > functions that are necessary to do so in build-side code if they're not > already part of core guile or other imports. I think the current implementation quirks have almost nothing to do with my reasoning for #:absent-dependencies. It's probably true that, if it were more convenient to write phases transforming "package.json" generally, I probably wouldn't have looked into why so many existing packages were deleting the configure phase---but I think it's probably also true that, if that had been the case, those packages wouldn't currently be deleting the configure phase. Part of my point is that, even if those utility functions did exist, I would still advocate for #:absent-dependencies. Jelle's latest email covers much of my reasoning, particularly this: On 12/20/21 18:10, Jelle Licht wrote: > Liliana Marie Prikler <liliana.prikler@gmail.com> writes: >> 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? On a similar note, as Jelle said elsewhere, I think #:absent-dependencies should be more amenable to programmatic code generation and transformation than a phase under `modify-phases` would be. The downside of having lambda, of course, is that some analysis is intractable or undecidable on Turing-complete (Church-complete?) code: there is a tradeoff to expressive power. > >> It seems like much more discussion would be needed on what the >> improvements should be, and potentially coordination with other users >> of (guix build json). Personally, I'd want to represent JSON objects >> with a real immutable dictionary type that gave us more guarantees >> about correctness by construction. If we continue with tagged >> association lists, we should write a little library of purely >> functional operations on JSON objects. But that all seems very far >> afield. > I'll have to say non sequitur to that. The functionality we require to > efficiently rewrite JSON can perfectly be built on top of (a)list > primitives and pattern matching, both of which are available to be used > in build-side code. We could even throw in SXML if we needed, not that > we do. There is really no need to code up yet another set of JSON > primitives just to write "hello, world". The other part of my point is that I think providing a nice set of utilities for more general JSON transformation is important enough that it should not be thrown into this patch series as an afterthought. The design is the hard part, not the code. If it's useful as an illustration (maybe it's not ...), here's a non-quirky implementation (not tested) of the interesting parts of `patch-dependencies` in Racket, with (other than the actual IO) exclusively pure functions operating on immutable data structures, which would not eliminate the benefits of `#:absent-dependencies`: ``` (λ (index absent-dependencies in out) (define (resolve-dependencies orig-deps) (for/hasheq ([{k v} (in-immutable-hash orig-deps)] #:unless (memq k absent-dependencies)) (values k (hash-ref index k v)))) (define (resolve-package-meta package-meta) (hash-update (hash-update package-meta 'devDependencies resolve-dependencies #hasheq()) 'dependencies (λ (orig-deps) (resolve-dependencies (hash-union orig-deps (hash-ref package-meta 'peerDependencies #hasheq())))) #hasheq())) (write-json (resolve-package-meta (read-json in)) out)) ``` To have a similarly pleasant API---I think we could aspire to an even better one!---the absolute minimum we'd need is a non-destructive `assoc-set`, since apparently that doesn't exist: it is, admittedly, fairly trivial. To be really convenient, though, we'd probably want `assoc-update`. But should `assoc-update` have an implicit default of `#f`, like `assoc-ref`, or raising an error, like Racket's `hash-update`/`hash-ref`/`dict-update`/`dict-ref`? Should we borrow the Racket convention whereby the "default value" can be a thunk (in which case it is called, so it could escape or build up some expensive-to-compute default value)? Or maybe `assoc-update` should take four mandatory arguments, with the update procedure last, because it will often be a long lambda expression, and the code might look more beautiful with that argument last. Maybe the default should be passed as a keyword argument! Then there's `assoc-set*`, `assoc-has-key?`, `assoc-delete`, `assoc-union` ... But wait! A big source of repetitive boilerplate is wrapping and unwrapping JSON objects with the '@ tag. If we're implementing all of these functions anyway, why not effectively compose them with: > > If you absolutely require it, though: > (define json-object->alist cdr) > (define (alist->json-object alist) (cons '@ alist)) so we can just write `jsobject-ref`, `jsobject-set`, `jsobject-delete`, `jsobject-update`, etc. directly? Insert bikeshedding about names here. But wait! Today, (guix build json) is not part of node-build-system's public API. It is not even part of the default value for #:modules (though it is in #:imported-modules). If we are ever going to change the JSON representation we use, surely we should consider it before making it public. There is a commit [1] in the history changing to use guile-json: do the reasons it was reverted "for now" [2] in 2019 still apply? And guile-json is a deprecated alias: would we want guile-json-1, guile-json-3, or guile-json-4? [1]: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=8eb0ba532ebbebef23180e666e0607ea735f9c1a [2]: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=a4bb18921099b2ec8c1699e08a73ca0fa78d0486 ... and so forth. These questions do not strike me as trivially self-evident. I don't know what answers I'd come up with for all of them. Given that, even if we already had these utility functions, I still think #:absent-dependencies would be The Right Thing, I'm very reluctant to add a prerequisite of designing general "package.json" manipulation tools. -Philip
diff --git a/guix/build/node-build-system.scm b/guix/build/node-build-system.scm index dcaa719f40..b74e593838 100644 --- a/guix/build/node-build-system.scm +++ b/guix/build/node-build-system.scm @@ -73,26 +73,45 @@ (define* (patch-dependencies #:key inputs #:allow-other-keys) (define index (index-modules (map cdr inputs))) - (define (resolve-dependencies package-meta meta-key) - (fold (lambda (key+value acc) - (match key+value - ('@ acc) - ((key . value) (acons key (hash-ref index key value) acc)))) - '() - (or (assoc-ref package-meta meta-key) '()))) + (define (resolve-dependencies meta-alist meta-key) + ;; Given: + ;; - The alist from "package.json", with the '@ unwrapped + ;; - A string key, like "dependencies" + ;; Returns: an alist (without a wrapping '@) like the entry in + ;; meta-alist for meta-key, but with dependencies supplied + ;; by Guix packages mapped to the absolute store paths to use. + (match (assoc-ref meta-alist meta-key) + (#f + '()) + (('@ . orig-deps) + (fold (match-lambda* + (((key . value) acc) + (acons key (hash-ref index key value) acc))) + '() + orig-deps)))) (with-atomic-file-replacement "package.json" (lambda (in out) - (let ((package-meta (read-json in))) - (assoc-set! package-meta "dependencies" - (append - '(@) - (resolve-dependencies package-meta "dependencies") - (resolve-dependencies package-meta "peerDependencies"))) - (assoc-set! package-meta "devDependencies" - (append - '(@) - (resolve-dependencies package-meta "devDependencies"))) + ;; It is unsafe to rely on 'assoc-set!' to update an + ;; existing assosciation list variable: + ;; see 'info "(guile)Adding or Setting Alist Entries"'. + (let* ((package-meta (read-json in)) + (alist (match package-meta + ((@ . alist) alist))) + (alist + (assoc-set! + alist "dependencies" + (append + '(@) + (resolve-dependencies alist "dependencies") + (resolve-dependencies alist "peerDependencies")))) + (alist + (assoc-set! + alist "devDependencies" + (append + '(@) + (resolve-dependencies alist "devDependencies")))) + (package-meta (cons '@ alist))) (write-json package-meta out)))) #t)