| Message ID | 314a0ea4-a851-6642-0a59-d4c61d65c242@philipmcgrath.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | None | expand |
Am Samstag, dem 18.12.2021 um 17:55 -0500 schrieb Philip McGrath: > [T]he failure mode for CommonJS require (ES6 import may be at least > somewhat better) is much less clean than this seems to suggest. I think the less I know about JavaScript, the better I'll sleep at night. > Returning to my concrete example, my motivation for this has been > trying to set up a Guix System service to run the Webthings Gateway > (formerly Mozilla IoT).[1] (Currently I have it on a branch of my Guix > fork at [2]; since there are many yacks left to shave before it could > be upstreamed, I plan to turn it into a channel once it can build > against Guix master, i.e. once this patch series is applied.) I > discovered the problem with the missing node-debug only when webthings- > service caused webthings-gateway to load webthings-addon-zwave-adapter > at runtime. > (Those are both node-build-system packages, but there are also > webthings-addon-* packages in Python and Rust, and in principle any > language could be used.) The webthings-addon-zwave-adapter has > node-serialport as a package.json dependency. As I remember it, even > then, the lack of node-debug only caused a runtime error on a certain > code path, perhaps triggered by the presence or absence of Z-Wave > adapter hardware: I had used all of the packages without problems > before hitting the issue. There was a stack trace, and it did help > 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? > In particular, we don't have any JavaScript test frameworks packaged > for Guix yet, so most of the time we aren't actually running the code > in our Node.js packages, just putting the right files in the right > places. So the runtime error may not come until some application/tool > has been created, potentially very far along the dependency graph, and > may not be revealed by tests (that we can actually run) at all. The fix to not having test frameworks is adding them, not > I think we should optimize for the kind of high-quality packages we > aspire to have in mainline Guix, where eventually we hope to have all > sufficiently useful libre Node.js packages available. In that case, > #:absent-dependencies makes it explicit when Guix packagers are > overriding an upstream decision. While we work to achieve that reality, > I think #:absent-dependencies is a much better place for a to-do list > than having to search build logs or source "package.json"s. However... 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 can see the use of a "warn" mode for hacking things together quickly > without having to totally delete configure. I think this could coexist > with #:absent-dependencies and could be done in a follow-on to this > patch series. > > Right now, the #:absent-dependencies argument must be a list of > strings. To support a "warn" mode, we could loosen that contract a bit: > we can bikeshed about details, but let's say that we're in "warn" mode > if the list contains the symbol 'infer. We change this code: > > --- > > diff --git a/guix/build/node-build-system.scm > b/guix/build/node-build-system.scm > index b74e593838..892104b6d2 100644 > --- a/guix/build/node-build-system.scm > +++ b/guix/build/node-build-system.scm > @@ -69,7 +69,8 @@ (define (list-modules directory) > input-paths) > index)) > > -(define* (patch-dependencies #:key inputs #:allow-other-keys) > +(define* (patch-dependencies #:key inputs absent-dependencies > + #:allow-other-keys) > > (define index (index-modules (map cdr inputs))) > > @@ -86,7 +87,9 @@ (define (resolve-dependencies meta-alist meta-key) > (('@ . orig-deps) > (fold (match-lambda* > (((key . value) acc) > - (acons key (hash-ref index key value) acc))) > + (if (member key absent-dependencies) > + acc > + (acons key (hash-ref index key value) acc)))) > '() > orig-deps)))) > > to do something like this: > > --8<---------------cut here---------------start------------->8--- > (if (or (member key absent-dependencies) > (and (memq 'infer absent-dependencies) > (not (hash-ref index key #f)))) > acc > (acons key (hash-ref index key value) acc)) > --8<---------------cut here---------------end--------------->8--- You're actively making the code that resolves dependencies worse to look at only to keep the keyword. Don't. There are tools besides a hammer. > Would that meet your objective? No. As I repeatedly pointed out, I want either no keyword addition for this "feature" or a keyword that can be regarded as essentially boolean if not outright implemented as one. Reading this again, the existing lines already do what I want (hash-ref index key value) spits out value as a default if key is not found. So the solution is to not touch patch-dependencies at all; we don't have to abuse a function that's not meant for sanity checking to check sanity. > It at least includes files listed in ".npmignore", but I think there > are entries in "package.json" that control the behavior, too. We should > adjust our repack phase to ignore those files---but I, at least, would > need to look into it further to know exactly what the correct behavior > is. Fair enough, that is probably out of scope for now, but we should revisit it before importing the node world and changing node-build- system becomes a core-updates task. > 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. 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. 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?
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? I don't understand the scenario you have in mind here. As I remember/understand the left-pad debacle, the package was deleted from the NPM registry by its author. I don't understand how that would require changes to Guix package definitions. Hopefully, Software Heritage would save the day. Perhaps Guix would choose to provide an alternate, compatible package implementing that single, trivial function, but we could do that by just changing the definition of the hypothetical Scheme variable `node-left-pad`. Eventually, downstream packages would presumably make changes, but we'd have to address those changes anyway when updating those packages. 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. 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. 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. 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. >> I think we should optimize for the kind of high-quality packages we >> aspire to have in mainline Guix, where eventually we hope to have all >> sufficiently useful libre Node.js packages available. In that case, >> #:absent-dependencies makes it explicit when Guix packagers are >> overriding an upstream decision. While we work to achieve that reality, >> I think #:absent-dependencies is a much better place for a to-do list >> than having to search build logs or source "package.json"s. However... > 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. 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. 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. >> I can see the use of a "warn" mode for hacking things together quickly >> without having to totally delete configure. I think this could coexist >> with #:absent-dependencies and could be done in a follow-on to this >> patch series. >> --8<---------------cut here---------------start------------->8--- >> (if (or (member key absent-dependencies) >> (and (memq 'infer absent-dependencies) >> (not (hash-ref index key #f)))) >> acc >> (acons key (hash-ref index key value) acc)) >> --8<---------------cut here---------------end--------------->8--- > You're actively making the code that resolves dependencies worse to > look at only to keep the keyword. Don't. There are tools besides a > hammer. > >> Would that meet your objective? > No. As I repeatedly pointed out, I want either no keyword addition for > this "feature" or a keyword that can be regarded as essentially boolean > if not outright implemented as one. > > Reading this again, the existing lines already do what I want (hash-ref > index key value) spits out value as a default if key is not found. So > the solution is to not touch patch-dependencies at all; we don't have > to abuse a function that's not meant for sanity checking to check > sanity. 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.) >> 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. > 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. > > 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. 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") ``` 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. 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? -Philip
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. > I don't know what further steps to take to resolve this disagreement > or how some decision ultimately gets made. > > Maybe someone else could weigh in on how to proceed? I’m probably not “someone else” enough at this point, but I guess we can ask the maintainers to weigh in/help facilitate. We try to move forward by consensus, and maybe replacing the keyword with a phase-making procedure will get us there. Liliana, what do you say? Have we found an approach we can agree on? If not, I think that we’re probably stuck and will need some fresh voices to move forward. -- Tim
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. > 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. > 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" [...])) > 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. > > 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'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. > > 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 :) > 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 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. > 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 :)
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")))))) although delete-dependencies could even be some chain of alist rewriting procedures if we wanted to be super evil. I don't think we would need to generate phases through FP, we can write them as code. > > I don't know what further steps to take to resolve this > > disagreement or how some decision ultimately gets made. > > > > Maybe someone else could weigh in on how to proceed? > > I’m probably not “someone else” enough at this point, but I guess we > can ask the maintainers to weigh in/help facilitate. We try to move > forward by consensus, and maybe replacing the keyword with a phase- > making procedure will get us there. Liliana, what do you say? Have > we found an approach we can agree on? If not, I think that we’re > probably stuck and will need some fresh voices to move forward. I personally think phase making is out of scope and we need a solid foundation first. That said, if Philip does provide both that foundation and a good reason to have a phase-making procedure, I'm willing to strike a compromise. Cheers
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
Hi Jelle, Here's a short answer to one specific question: On 12/20/21 18:10, Jelle Licht wrote: > Liliana Marie Prikler <liliana.prikler@gmail.com> writes: >> Am Montag, dem 20.12.2021 um 14:33 -0500 schrieb Philip McGrath: >>> 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. One key difference between the proposed `#:strict? #f` and the current status quo of `(delete 'configure)` is that, at least as I've understood the proposal, the patch-dependencies phase would still remove the (implicitly detected) absent dependencies from the "package.json", so the configure phase would be able to run `npm install`. That would fix the problem I described back in <https://issues.guix.gnu.org/51838#13> (a month ago), and the native packages would successfully build. I think it would be a less good option for the reasons you state, and which I've argued for elsewhere, but it's important to be clear that it would solve the problem building these packages. Actually, in an ideal world, I would agree with Liliana that #:absent-dependencies ought to be out of scope for this patch series. However, the existing solution for the problem of missing dependencies---deleting the configure phase entirely---does not work for packages that need to build native add-ons. So, if I needed to solve the problem for the native add-on packages, I thought I should find a generally-applicable solution that, in my view, is an improvement for the existing packages as well. More soon, Philip
Hi all, 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 could live with the above as a compromise. My reservation regarding: > > (add-after 'patch-dependencies 'drop-junk > (lambda _ > (with-atomic-json-replacement "package.json" > (lambda (json) (delete-dependencies json '("node-tap")))))) > is that `with-atomic-json-replacement` would make (guix build json)'s representation a part of node-build-system's API, which it currently is not. For the reasons I detailed in my last email, I think that would open up a larger can of worms than it might seem. Liliana, I'm not entirely certain, but my impression from: >> I’m probably not “someone else” enough at this point, but I guess we >> can ask the maintainers to weigh in/help facilitate. We try to move >> forward by consensus, and maybe replacing the keyword with a phase- >> making procedure will get us there. Liliana, what do you say? Have >> we found an approach we can agree on? If not, I think that we’re >> probably stuck and will need some fresh voices to move forward. > I personally think phase making is out of scope and we need a solid > foundation first. That said, if Philip does provide both that > foundation and a good reason to have a phase-making procedure, I'm > willing to strike a compromise. was that you would still have objections to something like: >>> ``` >>> #:phases >>> (modify-phases %standard-phases >>> (add-after 'unpack 'delete-dependencies >>> (make-delete-dependencies-phase '("node-tap")))) >>> ``` unless it came together with code like `with-atomic-json-replacement` for more general "package.json" transformations. If that's the case, then I guess we should do as Jelle suggests: On 12/20/21 18:10, Jelle Licht wrote: > 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 > -Philip
Hi, excuse my brevity, but I'll be off to work soon. Am Montag, dem 20.12.2021 um 22:59 -0500 schrieb Philip McGrath: > My reservation regarding: > > > > > (add-after 'patch-dependencies 'drop-junk > > (lambda _ > > (with-atomic-json-replacement "package.json" > > (lambda (json) (delete-dependencies json '("node-tap")))))) > > > > is that `with-atomic-json-replacement` would make (guix build json)'s > representation a part of node-build-system's API, which it currently > is not. For the reasons I detailed in my last email, I think that > would open up a larger can of worms than it might seem. That might be a valid concern, but I'd point to the "we don't gratuitously change API" shield. Plus if we do, we'd replace our current JSON by Guile-JSON. If that has a different internal representation that would awfully break things, please do tell. > I guess we should do as Jelle suggests: > > On 12/20/21 18:10, Jelle Licht wrote: > > I believe the best thing to do would be to push the earlier > > uncontroversial node patches. > > > >[...] I did suggest that too, but note that it would only upsteam patches 1-4 of 45, as patch 5 already touches node-build-system. Patch 5 would probably be fine to go as well (can the others confirm that?), but if the goal is to push today, someone else will have to do it, as I'll be only back at night. Cheers
Hi, On 12/21/21 00:20, Liliana Marie Prikler wrote: > Hi, > > excuse my brevity, but I'll be off to work soon. > > Am Montag, dem 20.12.2021 um 22:59 -0500 schrieb Philip McGrath: >> My reservation regarding: >> >>> >>> (add-after 'patch-dependencies 'drop-junk >>> (lambda _ >>> (with-atomic-json-replacement "package.json" >>> (lambda (json) (delete-dependencies json '("node-tap")))))) >>> >> >> is that `with-atomic-json-replacement` would make (guix build json)'s >> representation a part of node-build-system's API, which it currently >> is not. For the reasons I detailed in my last email, I think that >> would open up a larger can of worms than it might seem. > That might be a valid concern, but I'd point to the "we don't > gratuitously change API" shield. Plus if we do, we'd replace our > current JSON by Guile-JSON. If that has a different internal > representation that would awfully break things, please do tell. Here are, to the best of my understanding, the differences in representation among (guix build json) and the three versions of guile-json packaged in Guix. (For guile-json, this is based on my relatively-quick reading of the docs, not any direct experience.) An extra complication is that some part of Guix's code staging seems to incorrectly turn #nil into '(). I will see if I can narrow that down and file a bug report. (guix build json): • object -> (Pairof '@ (Listof (Pairof String Json))) • array -> (Listof Json) • null -> #nil • string -> String guile-json-4: • object -> (Listof (Pairof (U Symbol Number String) Json)) • array -> (Vectorof Json) • null -> 'null ;; configurable by keyword argument • string -> (U Symbol String) guile-json-3: • object -> (Listof (Pairof (U Symbol Number String) Json)) • array -> (Vectorof Json) • null -> #nil • string -> (U Symbol String) guile-json-1: • object -> (U (HashTable (U Symbol String) Json) (Listof (Pairof (U Symbol String) Json))) • array -> (Listof Json) • null -> #nil • string -> (U Symbol String) -Philip
Hi, Am Dienstag, dem 21.12.2021 um 13:25 -0500 schrieb Philip McGrath: > Here are, to the best of my understanding, the differences in > representation among (guix build json) and the three versions of > guile-json packaged in Guix. (For guile-json, this is based on my > relatively-quick reading of the docs, not any direct experience.) An > extra complication is that some part of Guix's code staging seems to > incorrectly turn #nil into '(). I will see if I can narrow that down > and file a bug report. > > (guix build json): > • object -> (Pairof '@ (Listof (Pairof String Json))) > • array -> (Listof Json) > • null -> #nil > • string -> String > > guile-json-4: > • object -> (Listof (Pairof (U Symbol Number String) Json)) > • array -> (Vectorof Json) > • null -> 'null ;; configurable by keyword argument > • string -> (U Symbol String) > > guile-json-3: > • object -> (Listof (Pairof (U Symbol Number String) Json)) > • array -> (Vectorof Json) > • null -> #nil > • string -> (U Symbol String) #nil to '() conversions are probably the fault of some syntax-case or match expression. That being said, I hope we don't have to worry about code staging too much as comparison ought to be done using null? imo. I think the main difference between (guix build json) and guile-json here are the extended keys in the latter (guix only has strings) and the vector/object distinction. I think it'd be rather straight-forward to write upgrade guidelines in case we ever make the switch. Similarly, all rules based on pattern-matching *will break*, forcing us to upgrade all the recipes with it. WDYT? Would that be manageable (assuming the change to require Guile-JSON would already be core- updates material)?
Hi, On 12/21/21 15:44, Liliana Marie Prikler wrote: > Am Dienstag, dem 21.12.2021 um 13:25 -0500 schrieb Philip McGrath: >> An >> extra complication is that some part of Guix's code staging seems to >> incorrectly turn #nil into '(). I will see if I can narrow that down >> and file a bug report. >> > #nil to '() conversions are probably the fault of some syntax-case or > match expression. That being said, I hope we don't have to worry about > code staging too much as comparison ought to be done using null? imo. I've reported the problem with g-expressions here: https://issues.guix.gnu.org/52749 In brief, the problem is that a Scheme value like: '(@ ("k" . #nil)) ought to produce the JSON: {"k":null} but, if it is part of a g-expression, it instead produces: {"k":[]} -Philip
On 12/21/21 15:44, Liliana Marie Prikler wrote: > Am Dienstag, dem 21.12.2021 um 13:25 -0500 schrieb Philip McGrath: >> Here are, to the best of my understanding, the differences in >> representation among (guix build json) and the three versions of >> guile-json packaged in Guix. > I think the main difference between (guix build json) and guile-json > here are the extended keys in the latter (guix only has strings) and > the vector/object distinction. For guile-json-4, the representation of the JSON value "null" is also different: #nil vs. the symbol 'null. > I think it'd be rather straight-forward > to write upgrade guidelines in case we ever make the switch. > Similarly, all rules based on pattern-matching *will break*, forcing us > to upgrade all the recipes with it. WDYT? Would that be manageable > (assuming the change to require Guile-JSON would already be core- > updates material)? > I actually might like (guix build json) better than guile-json-*! Using vectors for arrays seems roughly awkward as tagged alists, especially if Guile really doesn't have purely functional update procedures for alists---and I sure can't find any---so we'd have to write a little algebra of operations on JSON objects either way. But I'm not really familiar with the pros and cons of each, and I don't know the context behind the previous attempt to switch. My concern is that someone, or several someones, probably should know all of that context before exposing one representation or another as part of node-build-system's API. As I've said, I think that's a high-priority improvement! But it, and designing nice helper functions, seem quite far removed from the core purpose of this patch series. Since IMO #:absent-dependencies would still be The Right Thing even if those utilities already existed, and since #:absent-dependencies can be implemented without having to resolve any of the broader questions, I think it would be better not to entangle them with this patch series. Would it mitigate your concerns with #:absent-dependencies at all if I send a separate patch series adding more general utilities based on (guix build json)? -Philip
Am Donnerstag, dem 23.12.2021 um 00:19 -0500 schrieb Philip McGrath: > On 12/21/21 15:44, Liliana Marie Prikler wrote: > > Am Dienstag, dem 21.12.2021 um 13:25 -0500 schrieb Philip McGrath: > > > Here are, to the best of my understanding, the differences in > > > representation among (guix build json) and the three versions of > > > guile-json packaged in Guix. > > I think the main difference between (guix build json) and guile- > > json here are the extended keys in the latter (guix only has > > strings) and the vector/object distinction. > > For guile-json-4, the representation of the JSON value "null" is also > different: #nil vs. the symbol 'null. Different, but configurable, i.e. you can make guile-json-4 return #nil, which is what would be needed for the sake of compatibility. > > > I think it'd be rather straight-forward to write upgrade guidelines > > in case we ever make the switch. Similarly, all rules based on > > pattern-matching *will break*, forcing us to upgrade all the > > recipes with it. WDYT? Would that be manageable (assuming the > > change to require Guile-JSON would already be core-updates > > material)? > > > > I actually might like (guix build json) better than guile-json-*! > Using vectors for arrays seems roughly awkward as tagged alists, > especially if Guile really doesn't have purely functional update > procedures for alists---and I sure can't find any---so we'd have to > write a little algebra of operations on JSON objects either way. But > I'm not really familiar with the pros and cons of each, and I don't > know the context behind the previous attempt to switch. I don't think there's been any. Note that Guix already uses guile- json, just not at build side. I don't think there's only one reason for this; of course we have the incompatibility, but guile-json would probably also pull in much more into the build than would be needed in most cases, so that's that. For the medium/long term I do think guile-json will eventually go the emacs-json route of making everything about its (de)serialization configurable, which ought to ease the porting of guix build to it. However, pointing at the API stability shield, I don't think we'll be too hasty porting things over. > My concern is that someone, or several someones, probably should know > all of that context before exposing one representation or another as > part of node-build-system's API. As I've said, I think that's a > high-priority improvement! But it, and designing nice helper > functions, seem quite far removed from the core purpose of this patch > series. You would only have to implement helper functions as needed, i.e. for now just (with-atomic-json-replacement proc) and (delete-dependencies json). My personal interpretation is that read-json/write-json are already public Guix API and you can easily use them inside node-build- system as-is (note: you do need to use-modules them, but it doesn't require building another module closure), but perhaps someone else has a different reading. As for other helper functions, that might become useful over time, those can be added as-needed. > Since IMO #:absent-dependencies would still be The Right Thing even > if those utilities already existed, and since #:absent-dependencies > can be implemented without having to resolve any of the broader > questions, I think it would be better not to entangle them with this > patch series. I agree, that we can both agree, that whatever we had in mind when starting this discussion can be implemented without these primitives, but I disagree on it being The Right Thing. Particularly, in the case of #:absent-dependencies, simplicity and completeness are lacking, whereas in my original suggestion correctness and completeness were lacking. For the record, I don't think we can get to The Right Thing without exposing the JSON somehow. However, since you argue we can't, we need to find a course of action, that will allow us to do The Right Thing later. I'm sadly a little out of options as I write this, but open to some suggestions and perhaps I'll get another idea if I gaze at my navel for some time. > Would it mitigate your concerns with #:absent-dependencies at all if > I send a separate patch series adding more general utilities based on > (guix build json)? That series would block this one in my opinion (aside from the node changes, that could be done without it, but we're talking five patches at most here and I plan to do them over the holidays). This would sound like a good idea if you could wait for that discussion to be resolved, but I fear that's not the case, is it? Cheers
I think the #:absent-dependencies pattern is very straightforward argument and is clearly a common enough occurrence to merit a shorthand expression. I have not been following the developments in the node-build-system here recently but it looks like lots of progress is being made. Thanks all for contributing! -- Sincerely, Ryan Sundberg On 12/29/21 11:38 PM, Philip McGrath wrote: > Hi Liliana (and everyone), > > Thanks for taking care of the earlier patches from the series! > > It sounds like (guix build json) is going to be around for a reasonably > long time, and I'm ok with that. > > In the hope of clearing a path to move forward, I'm sending v6 of this > patch series, immediately followed by a closely-related v7: I strongly > prefer v7, but I would be ok with either version being applied if one of > them can achieve consensus. (If v6 is merged, I'll send a separate patch > series to propose '#:absent-dependencies'.) > > I've put the two versions up on GitLab > as <https://gitlab.com/philip1/guix-patches/-/tags/guix-issue-51838-v6> > and <https://gitlab.com/philip1/guix-patches/-/tags/guix-issue-51838-v7>, > respectively. > > I've re-organized the patches in the series somewhat to facilitate a > minimal, side-by-side comparison between '#:absent-dependencies' and this > approach: > > On 12/20/21 17:00, Liliana Marie Prikler wrote: >> (add-after 'patch-dependencies 'drop-junk >> (lambda _ >> (with-atomic-json-replacement "package.json" >> (lambda (json) (delete-dependencies json '("node-tap")))))) > > The series is now organized as follows: > > - The first 4 patches are identical in v6 and v7: > > - Patches 01 & 02/41 are non-controversial build system changes > ('delete-lockfiles' and libuv). > > - Patch 03/41 adds to (guix build node-build-system) several utility > functions for transforming JSON in the representation used by (guix > build json), especially functional update of tagged JSON > objects. It also adjusts the rest of (guix build node-build-system) > to make use of those functions, eliminating all uses of > 'assoc-set!' and other procedures that mutate assosciation lists. > > Of the new functions, only 'with-atomic-json-file-replacement' is > exported for now: my hope is that further (valuable and important!) > discussion about the API design of these functions or their > implementations need not block either v6 or v7 of this series. > > - Patch 04/41 adds the 'avoid-node-gyp-rebuild' phase. I've re-worked > the implementation to use the new helper functions. > > - Patch 05/41 is the truly significant difference between v6 and v7: it > implements the strategy for dealing with missing dependencies. > > In v6, it exports a new public function 'delete-dependencies' from > (guix build node-build-system). > > In v7, it adds '#:absent-dependencies'. One difference from previous > versions of this series is that it handles '#:absent-dependencies' in a > new 'delete-dependencies' phase, which makes the change even more > self-contained. > > - Patches 06 through 17/41 adjust existing packages to make use of the > approach to missing dependencies from patch 05/41 of each series. > > - Patches 18 through 41/41 add the new packages excercising native addon > support, again differing based on the approach from patch 05/41. The > other slight difference from previous versions of this series is that, > where applicable, I've adjusted the new package definitions to use > 'with-atomic-json-file-replacement' and never to use 'assoc-set!' or > other procedures that mutate assosciation lists. > > I continue to think that '#:absent-dependencies' is a good approach, in > brief, as Jelle put it in <https://issues.guix.gnu.org/51838#247>, because: > > On 12/20/21 18:10, Jelle Licht wrote: >> Liliana Marie Prikler <liliana.prikler@gmail.com> writes: >>> Am Montag, dem 20.12.2021 um 14:33 -0500 schrieb Philip McGrath: >>>> 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? >> > > Additionally, I think having a phase in '%standard-phases' is a good way of > addressing the need to use 'delete-dependencies' after the > 'patch-dependencies' phase, which I explain in patch v6 05/41. > > But, again, I could live with either v6 or v7 being applied if one of them > can achieve consensus. > > So, without further ado, here is v6! > > -Philip > > Philip McGrath (41): > guix: node-build-system: Add delete-lockfiles phase. > guix: node-build-system: Add implicit libuv input. > guix: node-build-system: Add JSON utilities. > guix: node-build-system: Add avoid-node-gyp-rebuild phase. > guix: node-build-system: Add 'delete-dependencies' helper function. > gnu: node-semver-bootstrap: Use 'delete-dependencies'. > gnu: node-ms-bootstrap: Use 'delete-dependencies'. > gnu: node-binary-search-bootstrap: Use 'delete-dependencies'. > gnu: node-debug-bootstrap: Use 'delete-dependencies'. > gnu: node-llparse-builder-bootstrap: Use 'delete-dependencies'. > gnu: node-llparse-frontend-bootstrap: Use 'delete-dependencies'. > gnu: node-llparse-bootstrap: Use 'delete-dependencies'. > gnu: node-semver: Use 'delete-dependencies'. > gnu: node-wrappy: Use 'delete-dependencies'. > gnu: node-once: Use 'delete-dependencies'. > gnu: node-irc-colors: Use 'delete-dependencies'. > gnu: node-irc: Use 'delete-dependencies'. > gnu: Add node-inherits. > gnu: Add node-safe-buffer. > gnu: Add node-string-decoder. > gnu: Add node-readable-stream. > gnu: Add node-nan. > gnu: Add node-openzwave-shared. > gnu: Add node-addon-api. > gnu: Add node-sqlite3. > gnu: Add node-file-uri-to-path. > gnu: Add node-bindings. > gnu: Add node-segfault-handler. > gnu: Add node-ms. > gnu: Add node-debug. > gnu: Add node-serialport-binding-abstract. > gnu: Add node-serialport-parser-delimiter. > gnu: Add node-serialport-parser-readline. > gnu: Add node-serialport-bindings. > gnu: Add node-serialport-parser-regex. > gnu: Add node-serialport-parser-ready. > gnu: Add node-serialport-parser-inter-byte-timeout. > gnu: Add node-serialport-parser-cctalk. > gnu: Add node-serialport-parser-byte-length. > gnu: Add node-serialport-stream. > gnu: Add node-serialport. > > gnu/packages/node-xyz.scm | 1013 +++++++++++++++++++++++++++++- > gnu/packages/node.scm | 78 ++- > gnu/packages/zwave.scm | 64 ++ > guix/build-system/node.scm | 9 +- > guix/build/node-build-system.scm | 355 ++++++++++- > 5 files changed, 1464 insertions(+), 55 deletions(-) >
diff --git a/guix/build/node-build-system.scm b/guix/build/node-build-system.scm index b74e593838..892104b6d2 100644 --- a/guix/build/node-build-system.scm +++ b/guix/build/node-build-system.scm @@ -69,7 +69,8 @@ (define (list-modules directory) input-paths) index)) -(define* (patch-dependencies #:key inputs #:allow-other-keys) +(define* (patch-dependencies #:key inputs absent-dependencies + #:allow-other-keys) (define index (index-modules (map cdr inputs))) @@ -86,7 +87,9 @@ (define (resolve-dependencies meta-alist meta-key) (('@ . orig-deps) (fold (match-lambda* (((key . value) acc) - (acons key (hash-ref index key value) acc))) + (if (member key absent-dependencies) + acc + (acons key (hash-ref index key value) acc)))) '() orig-deps))))