From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp2 ([2001:41d0:2:bcc0::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms0.migadu.com with LMTPS id oJcuOispvmElmAAAgWs5BA (envelope-from ) for ; Sat, 18 Dec 2021 19:32:11 +0100 Received: from aspmx1.migadu.com ([2001:41d0:2:bcc0::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp2 with LMTPS id ONHeNSspvmEHWQAAB5/wlQ (envelope-from ) for ; Sat, 18 Dec 2021 18:32:11 +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 858E2146B2 for ; Sat, 18 Dec 2021 19:32:11 +0100 (CET) Received: from localhost ([::1]:53182 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1myeV7-0003bT-PG for larch@yhetil.org; Sat, 18 Dec 2021 13:32:09 -0500 Received: from eggs.gnu.org ([209.51.188.92]:53954) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1myeV0-0003bC-Ug for guix-patches@gnu.org; Sat, 18 Dec 2021 13:32:02 -0500 Received: from debbugs.gnu.org ([209.51.188.43]:60498) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1myeV0-0004ij-M8 for guix-patches@gnu.org; Sat, 18 Dec 2021 13:32:02 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1myeV0-0007JY-Hd for guix-patches@gnu.org; Sat, 18 Dec 2021 13:32:02 -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: Sat, 18 Dec 2021 18:32:02 +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.163985231028098 (code B ref 51838); Sat, 18 Dec 2021 18:32:02 +0000 Received: (at 51838) by debbugs.gnu.org; 18 Dec 2021 18:31:50 +0000 Received: from localhost ([127.0.0.1]:43811 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1myeUn-0007J8-GM for submit@debbugs.gnu.org; Sat, 18 Dec 2021 13:31:50 -0500 Received: from mail-qv1-f43.google.com ([209.85.219.43]:45676) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1myeUl-0007Iv-Hj for 51838@debbugs.gnu.org; Sat, 18 Dec 2021 13:31:48 -0500 Received: by mail-qv1-f43.google.com with SMTP id js9so5492181qvb.12 for <51838@debbugs.gnu.org>; Sat, 18 Dec 2021 10:31:47 -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:from:subject:to:cc :references:content-language:in-reply-to:content-transfer-encoding; bh=40DTrNmW2Ec6QgyL8zWWwZ66DXCFxOwwxbkl8ujb1fY=; b=fCRgaqotResAJriYa/YotaEIZKCuXpnmWOg23yp/OXIRN/jJ/WAFxMUHeft4DHgkgQ I9QOKLKTrct4k676gpIUhVDB+Kpai7bHlcwkel9Nfw1HZMV4iyVDZmRzTwuNbVw57ltU Q9yx39+isuU20iAq4mAsdO6fV6rjtuZ7OiPdaIu+Ne1Ic4wTU9xwYy9cNKeIloYzPjdU cF9scVl09QvOI5V04DmXqS7BIaohS1HoCOvPfdkf4NDcPsTtoAl/5OplHmdNWAQYbi45 qdK8VqiY7qiem0I7sz6dnEDdLJmzmJoL4e2FttxYMmqN7455DiBRfzKiFGozOTKiGhhN LuzQ== 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:from :subject:to:cc:references:content-language:in-reply-to :content-transfer-encoding; bh=40DTrNmW2Ec6QgyL8zWWwZ66DXCFxOwwxbkl8ujb1fY=; b=EHKxcsUZWqieV2SnCWd1YHTROjlhoKTb+383JbOyKFyx2IGtg5lM3RbymQn7xj1n4U XW5wz+uy4zuG7ZXMh4mC80opQANiEdNIWmTPhXueznj1nnTCNAe2whSUo21pXeK8FwBm TjQQlh15L1qubK1OitL/WTaKkKUXIiHtwGGdigAbQxZvV5HNGkfgnfIIc5Un9FD6XYK6 R8pfVLJwb4oS/I69qaO70rOR3VSBcP+v1Ay+IHramksm4AurjTo/a+vYzwsBDSrNYf+A If5RRY+OTXLhvicij098nSOoBXv+c9XQyOEckgRQmipSnjztQZk91RnE0zfIWW1gZNIM TKRw== X-Gm-Message-State: AOAM532+Eov0ZQvP8+7wmsyjPHK8+jEhYkO9bzeZbLp7V6ci6k7Os/ia hoYWJNgYD80HGrdDoJ5Zepoong== X-Google-Smtp-Source: ABdhPJyicaohUk5e9YBK7c0dz0tz+HYvKBCwV/xvzoE2XSHSXH6hoJNEO5kDUZKSmL7i1V0vhcxuoA== X-Received: by 2002:a05:6214:1cc7:: with SMTP id g7mr7063399qvd.91.1639852301872; Sat, 18 Dec 2021 10:31:41 -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 8sm10650133qtz.28.2021.12.18.10.31.40 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 18 Dec 2021 10:31:41 -0800 (PST) Message-ID: <33696592-8a3a-8260-bf27-652cf78727a6@philipmcgrath.com> Date: Sat, 18 Dec 2021 13:31:40 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.3.1 From: Philip McGrath 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> Content-Language: en-US In-Reply-To: <815f327e36ecd24066179586997947ecd8f31150.camel@gmail.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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=1639852331; 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=40DTrNmW2Ec6QgyL8zWWwZ66DXCFxOwwxbkl8ujb1fY=; b=PwuL7Cs9B0A38CVjTwVQAOoXaoQCG6WgVm7urnyUJeKuUZDVhTL2jD1yA4DHNqSHbUYy59 vwhWCBn8m3e6wFZ4d2I/R/zQZpPnYxuuD0R1K05ynUPQu31Onn2ynkgX16i2utFFn00bH1 qs/+Ct9hzdNnm/zVKHy/HUw3GwhPfNhYkx7JC6pOSfxehI9/LReNglBbmheY5qy5+XKI+b aEqlsrRo5wsL4fNsc7W0iAKTkJvBN1nlr1GrNlIOS7rthpqIYaH0pUgoykrbWzbKqKfe2a 8VknCyd3WTsjdjKvA0geQtvdHUy3x/j2rNK3utlZXpINWaUH0ChupvuhpdD0Pg== ARC-Seal: i=1; s=key1; d=yhetil.org; t=1639852331; a=rsa-sha256; cv=none; b=ueVwrEFFkylfNtJy8H49DYzEULq9/bm6YpaOpghHa69NHwo33GD6f8jJAoaK1lIuHVxcH1 /N1+OoGYX6U1S2yXCwSpfDsDdGkJ3gV4oVkov9CzwyGwqrw08+J4aUG80V8kxgv1FIoiZm jyy2w3uYy2gUoSSpMooUPmstuSta0DoFxdMKyOQqYHAPzSYkPfzCU2O42U09BjthZnOg65 CBvM8PPymkE8ieVNkjeFyKZ1pHCTiOMs688+BPf21ibr1Zol0Mtz8pEvpdhWdpi3EWK4Em fHw/seOuI4s6TA86ci8hu8rqKtHWAn04amqNZsPkq1N6ne9N55OInlqUop1hww== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=philipmcgrath.com header.s=google header.b=fCRgaqot; 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: -1.81 Authentication-Results: aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=philipmcgrath.com header.s=google header.b=fCRgaqot; 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: 858E2146B2 X-Spam-Score: -1.81 X-Migadu-Scanner: scn0.migadu.com X-TUID: Er6dTYiCSGlo Hi, On 12/18/21 03:30, Liliana Marie Prikler wrote: > Hi, > > Am Freitag, dem 17.12.2021 um 21:48 -0500 schrieb Timothy Sample: >> Liliana Marie Prikler writes: >> >>> For the GNU build system (and likewise meson-build-system), the >>> default behaviour if you haven't specified anything as per upstream >>> conventions is typically to error if the package is required and >>> omit it if it's not.  The default behaviour of node-build-system >>> (and likewise cargo and most other build systems that come with the >>> advertisement of "we know package managers better than people who >>> actually produce usefulpackage managers) is "Oh my god, you don't >>> have an exact copy of the machine that built this stuff locally, I >>> am going to barf huge walls of noise at you".  Therefore, we can't >>> meaningfully compare those build systems in terms of strategies. >> >> NPM packages tend to wildly over-specify their dependencies.  We >> already remove dependency version checking, and before this change, >> many of our packages skipped any kind of dependency checking by >> skipping the configure phase altogether.  To me, the ‘#:absent- >> dependencies’ approach tries to work around the dependency over- >> specification by listing exactly those things that are only there to >> elicit a useless “Oh my god [...], I’m going to barf huge walls of >> noise” message.  The rest of the dependencies are those that the Guix >> package author deemed required (or at least important).  Basically, >> ‘#:absent-dependencies’ helps us translate between the NPM culture of >> over-specification (which is really a culture of prioritizing package >> author over package user) and the GNU culture of “DWIM” dependencies. > Except that it's not. The current workaround is "I know this thing's > going to barf at me, so I prepare an umbrella and hope it has no > holes". > >>> If we really want some static verification for node-build-system, I >>> think we should take that as an approach rather than hard-coding >>> (absent) dependencies literally everywhere. >> >> We need some way to know what to statically verify.  We can’t >> magically know what’s important and what isn’t.  The two options in >> this thread are ‘#:absent-dependencies’, and only checking what’s >> already in the package’s inputs.  What worries me about the second >> approach is that it offers no help when updating a package.  With >> ‘#:absent-dependencies’, if the developer adds a new dependency that >> really is required, we will get a build-time failure letting us >> know.  Whoever is updating the package can fix it before even >> committing anything.  If we just check the inputs, that’s not the >> case, and we might end up with Philip’s “mysterious runtime error, >> potentially many steps down a dependency chain.”  Hopefully tests >> would catch it, but I like the extra assurance. > That's why I didn't want to default to "do nothing", but to *warn* > about missing dependencies in configure. Then whoever bumps the > package will at least know which warnings are produced if they do so > and they can cross-check by manually building past versions. Including > #:absent-dependencies is no safe-guard against a failure here anyway. > A dependency that was optional in V1 might become required in V2. I also feel like I'm missing something, though maybe I just disagree. To try to be concrete, here's a real example. Between v3 and v4 of this patch series, I discovered that leaving out node-debug could actually cause runtime problems for some of the node-serialport packages. (It turns out it's really a logging library, which wasn't what I'd thought at first.) After I added the node-debug, I could easily search node-xyz.scm for the packages that listed it among their #:absent-dependencies and add it to them as an input. It seems like this would be much less convenient if node-build-system were to silently delete such dependencies and simply print a warning. I guess I would have to search through the build logs for all Node.js packages, right? More generally, I think truly "optional dependencies" (in the Node.js sense, to the extent I understand it) and dependencies we actively don't want (e.g. because node-sqlite3 shouldn't transitively depend on node-aws-sdk) are relatively rare cases. The most common cases seem to be dependencies we really would like to have, such as test frameworks or Typescript, but haven't packaged yet, and thus need to work around. Many packages that have #:absent-dependencies for such reasons also need to use `#:tests? #f` or to replace the build phase with some kind of manual alternative. I guess I don't understand what case the warn-and-drop approach is optimizing for. For both the case when dependencies aren't packaged for Guix yet and the case when Guix packagers have actively decided to skip some upstream dependencies, I think #:absent-dependencies is more useful. Having to look for that information in warnings in the build log seems much less ergonomic. >> Another benefit is that it would help us gain knowledge as to which >> NPM packages are often used but not actually required (e.g., NPM >> publishing tools).  With this knowledge, we could write a clever NPM >> importer that ignored obviously inessential dependencies. I agree with this, too: likewise, we could see packages that are often wanted but aren't in Guix and prioritize adding them! Part of the benefit of #:absent-dependencies, IMO, is as a form of communication with humans. >> I guess I’m starting to sound like a broken record now – this is >> basically what we covered before!  :)  Maybe we’re in need of a fresh >> perspective.  (If anyone is reading along and has thoughts, feel free >> to chime in!) > I think the NPM convention is to put everything you need "at build > time, but not at runtime" into dev-dependencies, no? In any case, one > approach I could offer is to sanity-check by searching for require() > statements and trying them in a controlled node environment. This > could look something like > > eval("try { var dep = require('" + dependency + "'); true } > catch (e) { false; }") > > Once we know where require statements are made and whether they > succeed, we can start estimating the impact of a missing dependency. > For this, it'd be nice to have a full function call graph, in which a > node is coloured dirty if it has a failing require statement, lies > within a module that has one or calls into a dirty node. However, as a > primitive approximation we can also count the node modules with failing > requires against those that don't. We set an arbitrary threshold of > allowed failures, e.g. 0.42, and then check that whatever value we > obtain from the above is lower than that. This could be interesting, and I think some of the JavaScript blunders we don't have packaged for Guix yet try to do something like this, but such analysis is not tractable in the general case, especially with CommonJS `require`, which is just a function and can be given arbitrary arguments computed at runtime. (And some packages really use it that way!) Also, currently node-build-system doesn't seem to be removing those files which `npm pack` is supposed to exclude, which would probably be a prerequisite for addressing this. > While that would be nice and all, I think the overall issue with the > current node implementation in Guix is that 'configure' and 'sanity- > check' are the same phase, so you have to disable both or none. I > think we could easily do with a configure phase that does nothing, not > even warn about a missing dependency, and a sanity-check phase that > requires every dependency mentioned in package.json to be met. > Packagers would then outright delete sanity-check as they do for python > and as they did for configure (but not have configure fail due to it!) > or deliberately rewrite the package.json for the sanity check and > dropping absent dependencies, i.e. what you do minus the keyword. If > later needed for the purposes of an importer, we would then still have > that database and could at some point introduce the key #:insane- > requirements. WDYT? I don't understand the benefit of this, and I'm also confused about the proposed implementation specifics. Why even have "a configure phase that does nothing"? What phase would run `npm install`? Presumably, we would have to delete all missing dependencies before that. I think there is room for improvement in node-build-system. One thing I've been thinking about is some articles I've seen (but not fully thought through yet) from the developers of PNPM, an alternative package manager for Node.js, which seems to have some similarities to Guix in symlinking things to a "store".[1][2][3] (It could be especially interesting for bootstrapping npm! And the approach to "monorepos" also seems relevant.) I also think an importer is very important, even if it's an imperfect one: `guix import npm-binary` was indispensable in developing these patches. I have some ideas about improving it, in particular that we should assume the newer "^" semantics for dependencies everywhere (i.e. that major versions and only major versions have incompatible changes: a common case recently seems to be moving from CommonJS modules to ES6 modules). As I understand it, node-build-system is undocumented, with no guarantees of compatibility. If #:absent-dependencies is at least an improvement over the status quo---which I think it is, since the new packages wouldn't build with the status quo---could we apply this and replace it later if someone implements a better strategy? I don't think I can implement control-flow analysis for JavaScript within the scope of this patch series. -Philip [1]: https://pnpm.io/blog/2020/05/27/flat-node-modules-is-not-the-only-way [2]: https://pnpm.io/symlinked-node-modules-structure [3]: https://pnpm.io/how-peers-are-resolved