On 2023-08-25 10:28, Andrew Tropin wrote: > On 2023-08-22 18:25, Ludovic Courtès wrote: > >> Hi Andrew, >> >> Andrew Tropin skribis: >> >>> Sorry for comming late to the party, I saw this message only a week ago >>> and didn't have time to make an extensive reply yet, so I will share my >>> quick thought on the most problematic part and maybe later will >>> formulate others thoughts in more details. >>> >>> define-service-type-mapping looks imperative and potentially very >>> problematic. Collecting those values in unknown order and applying this >>> implicit transformation making a good room for foot shooting. Imagine >>> someone would like to use his own (let's say) shepherd home service >>> implementation and will add this to one of the source files of their >>> channel: >>> >>> (define-service-type-mapping >>> shepherd-root-service-type => my-home-shepherd-service-type) >>> >>> What happens if somebody will use his channel just for getting some >>> package? Very likely it would break the build or in the worst case it >>> will build with unexpected service implementation under the hood. >> >> Yes, this is always possible. Though it’s not very different from: >> >> (set! home-shepherd-service-type …) > > Yes, and and this is exactly what this solution does and in addition to > that it hides it under the sweet define-service-type-mapping interface. > `set!` explicitly communicates the danger of usage, > define-service-type-mapping does not. > > > 1. We introduce a global state here and infect potentially all the > modules from all channels with it + mask it with nice interface. => To support my previous suggestion to revert this patch series I will document the example of a consequence: https://git.savannah.gnu.org/cgit/guix.git/commit/?h=cf6abf50dbbbd95fef465ab4bb3b608843ac47e1 https://issues.guix.gnu.org/65510 It highlights this: > Now the result of evaluation depends on the order source files are > read. >>> Collecting those values in unknown order and applying this >>> implicit transformation making a good room for foot shooting. to say it differently this change introduced the implicit dependency on the fact of the evaluation of the form, which is not explicitly referenced anywhere. > Every channel can break valid user's configurations with perfectly > legal public guix API. We make reloading of the modules and > REPL-driven development in general a huge pain in the lower back. > > 2. The service extension mechanism is already quite complex to understand > on its own, in addition to that the devirsity of record creation macros > / DSLs (define-record-type, define-record-type*, define-configuration) > doesn't make the situation better. We introduce one more DSL on top of > couple existing. => Learning curve raises even higher. Inspecting, > navigating and debugging such code becomes harder. It prevents future > extension with for-container or for-development-environment. It saves > couple of lines and avoids some minor repetions at the moment, but not > sure that it's the right way to reuse the stuff between home and system > services. > >> >> Maybe the unintended effect is more likely to happen unwillingly though, >> maybe. >> >> Do you have other solutions in mind, be it for this specific issue or >> for system/home service mapping in general? >> >>> I had [1][2] and still have concerns about macros and records >>> composability and reusability. I personally don't like excessive usage >>> of them in general. By adding more macros, already quite complex guix >>> services mechanism becomes even more harder to learn, inspect, reason >>> about and work with. In addition to that it has a major technical issue >>> mentioned above. I'm strongly against this change and would suggest to >>> revert it. >>> >>> I hope it doesn't sound rude and I'm really thankful for your work on >>> this, but I just think it's not the right solution, at least yet, in its >>> current form. >> >> It does sound a bit rude. :-) I would have loved to get any feedback >> from you while we were discussing this in the course of reviewing the >> Syncthing and Dicod patches a couple of months ago (which I believe you >> were Cc’d on, as member of the Home team). >> >> I won’t argue about macros and records, it’s off-topic: macros and >> records are part of the Schemer’s toolbox, we try and use them wisely, >> and Guix code has always used macros and records. We can discuss >> whether a specific macro or record type is suitable, of course, but >> general statements about them are unhelpful. >> > > Actually, it's quite on-topic IMO and it seems as a consequence of the > abandoned discussions I linked earlier, but it's a long story and we can > keep it aside for now, because despite the reasons lead to this solution > it has fundamental problems on its own we better to focus on. Maybe > later I'll elaborate on my thought process in more details. > >> Was it ‘for-home’ that triggered your comment? >> >> The patches do not introduce any new record type IIRC; what triggered >> your comment regarding records? >> >> I’m all for making changes to improve on this patch series. I’m against >> reverting the patch series: the conditions for reverting are not met >> (info "(guix) Commit Access"). >> >> Thanks for your feedback, >> Ludo’. > > Please treat it with adjustment to my not very proficient level of > english and lacking ability to express nuances. It's not a personal > critique in any way, it's my roughly formulated concerns on 1. technical > aspects of the solution and 2. possible consequences for the future of > guix and its user-facing API. -- Best regards, Andrew Tropin