From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp0 ([2001:41d0:8:6d80::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms0.migadu.com with LMTPS id KLnsE+nawGHr2wAAgWs5BA (envelope-from ) for ; Mon, 20 Dec 2021 20:35:05 +0100 Received: from aspmx1.migadu.com ([2001:41d0:8:6d80::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp0 with LMTPS id cOKTD+nawGFudgAA1q6Kng (envelope-from ) for ; Mon, 20 Dec 2021 19:35:05 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id DFFE42C45F for ; Mon, 20 Dec 2021 20:35:04 +0100 (CET) Received: from localhost ([::1]:48308 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mzOR4-0000HP-LO for larch@yhetil.org; Mon, 20 Dec 2021 14:35:02 -0500 Received: from eggs.gnu.org ([209.51.188.92]:58738) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mzOQ5-0000A5-Vz for guix-patches@gnu.org; Mon, 20 Dec 2021 14:34:02 -0500 Received: from debbugs.gnu.org ([209.51.188.43]:39952) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mzOQ5-0003xn-MA for guix-patches@gnu.org; Mon, 20 Dec 2021 14:34:01 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mzOQ5-0001vv-JH for guix-patches@gnu.org; Mon, 20 Dec 2021 14:34:01 -0500 X-Loop: help-debbugs@gnu.org Subject: [bug#51838] [PATCH v5 07/45] guix: node-build-system: Add #:absent-dependencies argument. Resent-From: Philip McGrath Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Mon, 20 Dec 2021 19:34:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 51838 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: Liliana Marie Prikler , Timothy Sample Cc: 51838@debbugs.gnu.org, Pierre Langlois , Jelle Licht Received: via spool by 51838-submit@debbugs.gnu.org id=B51838.16400288247403 (code B ref 51838); Mon, 20 Dec 2021 19:34:01 +0000 Received: (at 51838) by debbugs.gnu.org; 20 Dec 2021 19:33:44 +0000 Received: from localhost ([127.0.0.1]:51498 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mzOPn-0001vL-TS for submit@debbugs.gnu.org; Mon, 20 Dec 2021 14:33:44 -0500 Received: from mail-qv1-f45.google.com ([209.85.219.45]:37479) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mzOPl-0001v6-I1 for 51838@debbugs.gnu.org; Mon, 20 Dec 2021 14:33:42 -0500 Received: by mail-qv1-f45.google.com with SMTP id fo11so10354813qvb.4 for <51838@debbugs.gnu.org>; Mon, 20 Dec 2021 11:33:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=philipmcgrath.com; s=google; h=message-id:date:mime-version:user-agent:subject:content-language:to :cc:references:from:in-reply-to:content-transfer-encoding; bh=P8ZVz+MSU/Avll4FN9IcAHyBG0uutLNxsrF1YfAy3Vg=; b=gqJYkFxhUWYqIFlkcrwrTdNmlPrbrCdwpLiTvzIzGmxHQmlriPLMvBTgN+JPLaXRqn xiKKBX2CrFLKYFn1VSnEWc2HYNjQ5mTmbtVZ4hKugnXzH+dLp9eEvlCjTF7RtQBaHKgT ye/9ynep2K1cIkKT1BFRnaEY29IPiZdF0rHVzqXjFU61iptINUVgM1Na/hENkwZSQEX7 Kc0QGKRxrcmIPt6ntV6SAJAGCMpyWH6IGh3KxPmMqUJQVawU6Yc8NE0PY5ReV/pGhj82 JPIZZTh2GFqSk0Jyec5hkVG+exOyVIgvrK/Ow7+nXsLBQpWUTlqkame4vzkGZqAdadHC pPfQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=P8ZVz+MSU/Avll4FN9IcAHyBG0uutLNxsrF1YfAy3Vg=; b=sEzOr30XNi/yky/C5NpbEdO98+EXscYliIyrllqoOYqrReJTS24T98mgNjaOb8nJFW /kdQLNoLx3UAAZmC6w/Jdhzjxm4SCugMaureNTsjflGgxoc6Q7vIJ/LXhQHfkrsdLTxr FtsqSEIyrHs34apmOyX5kvvv/sYmyo5Qcubj9kuvkKWRSibQIX2hdHvKqlA1+VfRNZDw 2mj+09znsJqOFirEZpjfTaeEXFanhqGGVyo9iJYem/lJoe1X4qk2d7d/JoPY2d7/PJam 5sINjMzZ/VG1GaNFZl3PlZApdeau3UjpDuop38im/QTolRSyFGUreFRbaOo7KQwyUZQ9 h4Gg== X-Gm-Message-State: AOAM5314sTqtfG6LjXY6iSgMQnEClblBD1PqbkLTwYg4G2drMCKqA5RI DIYQiR1XJqMaCIpr8UUF6qmP12JmyTShF99F X-Google-Smtp-Source: ABdhPJx+uD9UjgNBhsNYF6AcirzRBMds2fcwD0igSM+iJ0MJJulcY1PGhqZsbfsfAwfbizqgyO+JWA== X-Received: by 2002:a05:6214:d66:: with SMTP id 6mr1135898qvs.85.1640028815953; Mon, 20 Dec 2021 11:33:35 -0800 (PST) Received: from [192.168.45.37] (c-73-125-89-242.hsd1.fl.comcast.net. [73.125.89.242]) by smtp.gmail.com with ESMTPSA id f8sm15634337qtk.1.2021.12.20.11.33.35 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 20 Dec 2021 11:33:35 -0800 (PST) Message-ID: <9533cfc8-b822-edc0-67ac-7e2c9aeacb09@philipmcgrath.com> Date: Mon, 20 Dec 2021 14:33:34 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.3.1 Content-Language: en-US References: <20211213060107.129223-1-philip@philipmcgrath.com> <20211217020325.520821-1-philip@philipmcgrath.com> <20211217020325.520821-8-philip@philipmcgrath.com> <87k0g36xp3.fsf@ngyro.com> <871r2a7hme.fsf@ngyro.com> <815f327e36ecd24066179586997947ecd8f31150.camel@gmail.com> <33696592-8a3a-8260-bf27-652cf78727a6@philipmcgrath.com> <2c803ea8f5c5c16c892f5528f63a4c7e98470c5b.camel@gmail.com> <314a0ea4-a851-6642-0a59-d4c61d65c242@philipmcgrath.com> <35164d79ab6ae1c0595d27f023376cf98ab1a39d.camel@gmail.com> From: Philip McGrath In-Reply-To: <35164d79ab6ae1c0595d27f023376cf98ab1a39d.camel@gmail.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+larch=yhetil.org@gnu.org Sender: "Guix-patches" X-Migadu-Flow: FLOW_IN X-Migadu-Country: US ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1640028905; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding:resent-cc: resent-from:resent-sender:resent-message-id:in-reply-to:in-reply-to: references:references:list-id:list-help:list-unsubscribe: list-subscribe:list-post:dkim-signature; bh=P8ZVz+MSU/Avll4FN9IcAHyBG0uutLNxsrF1YfAy3Vg=; b=hRSNZYxeOSdmmO+RS8QZnsBoR9i8EWGpXEiO+Tx4NcFnRWAbNiBg9k67cx9UP5KNsxp68b YaFOPxaSg0C7AVtoYzJkz9vm1FCFuXjUnAHw4kaRuzsvRFFwuzxQrOWS6g/Or0pkSjCGu6 W9PGieyN9OroY4d0CXxF8mpL5h6z90EYvgai8s8kTK4/D3SOYFxNIEMlp5SftdWFIbrRUX uw83tGPifCO3zlzT90PlFCuwR7fCRg9+991ER89ADxVtGx75jzbjQ82rguV2LH0tMaFcTP QjLenLE+IanCLnUx6mdrbKULVdwZU9/Qz2azy0xgeKcMl9rMyuUob8D+QpdRpw== ARC-Seal: i=1; s=key1; d=yhetil.org; t=1640028905; a=rsa-sha256; cv=none; b=ZRQvYFvqOS6YTRiUrtczLxI7cUxMvt7HpFx45qg2Cv6wfP1f8NNey7yXYjBGfeGUjd40bs 2P3FpwNExt08MhkFG06kK1AsAqWCAmM7HkSunmLEgqp+aTriM9KZUOLmaGjMxGDZZI3XPW Fu1LOwbvp0zblmWETbPmlCymyJHbzphc/xvycOPZHRGACgqUuMRlV82wnJ4IHYEVf8q730 K4dwUKhulODDqXHa/ha5p2sIZzltyItYJkTyeIV+XeOv+Biwzs32SKrApzQjZfAkp5nq4x EP89hvOMwwiolguGL4e3Y9GuGjEJ1HvO8vIzfaU1ybPBuyzrukr62B64VgB99A== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=philipmcgrath.com header.s=google header.b=gqJYkFxh; dmarc=none; spf=pass (aspmx1.migadu.com: domain of "guix-patches-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="guix-patches-bounces+larch=yhetil.org@gnu.org" X-Migadu-Spam-Score: 0.48 Authentication-Results: aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=philipmcgrath.com header.s=google header.b=gqJYkFxh; dmarc=none; spf=pass (aspmx1.migadu.com: domain of "guix-patches-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="guix-patches-bounces+larch=yhetil.org@gnu.org" X-Migadu-Queue-Id: DFFE42C45F X-Spam-Score: 0.48 X-Migadu-Scanner: scn0.migadu.com X-TUID: YKBgX3mE+cKr 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