From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp0 ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms0.migadu.com with LMTPS id GE4QAvEwpGCvCgEAgWs5BA (envelope-from ) for ; Tue, 18 May 2021 23:26:09 +0200 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp0 with LMTPS id eIRbOfAwpGCGZQAA1q6Kng (envelope-from ) for ; Tue, 18 May 2021 21:26:08 +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 7C83B1AE78 for ; Tue, 18 May 2021 23:26:08 +0200 (CEST) Received: from localhost ([::1]:51726 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lj7E7-0001hG-La for larch@yhetil.org; Tue, 18 May 2021 17:26:07 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:59056) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lj7E2-0001dH-88 for guix-patches@gnu.org; Tue, 18 May 2021 17:26:02 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:45937) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1lj7E2-0001vP-0K for guix-patches@gnu.org; Tue, 18 May 2021 17:26:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1lj7E1-00067s-T7 for guix-patches@gnu.org; Tue, 18 May 2021 17:26:01 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#47869] [PATCH core-updates] =?UTF-8?Q?=E2=80=98which=E2=80=99?= looks in PATH, incorrect when cross-compiling Resent-From: Maxime Devos Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Tue, 18 May 2021 21:26:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 47869 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: Ludovic =?UTF-8?Q?Court=C3=A8s?= Cc: 47869@debbugs.gnu.org Received: via spool by 47869-submit@debbugs.gnu.org id=B47869.162137314323523 (code B ref 47869); Tue, 18 May 2021 21:26:01 +0000 Received: (at 47869) by debbugs.gnu.org; 18 May 2021 21:25:43 +0000 Received: from localhost ([127.0.0.1]:57483 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1lj7Di-00067K-ER for submit@debbugs.gnu.org; Tue, 18 May 2021 17:25:43 -0400 Received: from michel.telenet-ops.be ([195.130.137.88]:53400) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1lj7Df-00067A-Us for 47869@debbugs.gnu.org; Tue, 18 May 2021 17:25:41 -0400 Received: from ptr-bvsjgyjmffd7q9timvx.18120a2.ip6.access.telenet.be ([IPv6:2a02:1811:8c09:9d00:aaf1:9810:a0b8:a55d]) by michel.telenet-ops.be with bizsmtp id 6MRe250020mfAB406MRe09; Tue, 18 May 2021 23:25:38 +0200 Message-ID: <343ead490dec84fec8694d2411963d3a80d27166.camel@telenet.be> From: Maxime Devos Date: Tue, 18 May 2021 23:25:28 +0200 In-Reply-To: <87v97f7oll.fsf_-_@gnu.org> References: <0892bdfbc097b07631190c8526a41d57b456d343.camel@telenet.be> <87v97f7oll.fsf_-_@gnu.org> Content-Type: multipart/signed; micalg="pgp-sha512"; protocol="application/pgp-signature"; boundary="=-JB+lpO7hFMU1O10sr8Bc" User-Agent: Evolution 3.34.2 MIME-Version: 1.0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=telenet.be; s=r21; t=1621373138; bh=iVxaxxkCmlVLx6BWnV6OGa74cKfA2qDJVM7nbzb7zCY=; h=Subject:From:To:Cc:Date:In-Reply-To:References; b=LLYhww3RsuzwN15vNGQ19bEKA5atnmzb2dm6BrRYBUngdn+mdJdmj8krHarGa01o0 XKHmeG3z3YfwzHU87pKmCUYI4mtskGynuSkUxOLdlNH0ux63BISw/MxU4q1TvPT1K8 KHIQKr48wnLsOClIXYk4HLvw3mCSi5fLdZ5pAkD/UNVndlRGobUpw3jAiNvD9PQtPm 2GczXjTUAlgQib/OZ+egMYnlllluRVnJIxULY5Iqzl4167k3vQpKeRjnVo0nKdWbrH jg6u0t8gZqO846+9ZtgBi5m8jamSROG3XMXc6v3SC8yC63XUJvhTNS4iB7ofQu4XBP ZFLvMn3//BIyg== 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 ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1621373168; 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: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=iVxaxxkCmlVLx6BWnV6OGa74cKfA2qDJVM7nbzb7zCY=; b=GdWA2QWSMPoYjycPUna/ZGH93DgA0YWwCk1nRxV+N/JiNWx2qXzIrVp3sVL06+7iDGkDAv LkhjPKydUk/SFH+mMv23ed+td4byrR23DGKxOU2i/BmwTQyoYs3AWaqOINCB+37pX/Pprc le2aUjz8GvFHSZq0BRtYL8p1mjI2T3F3UPvR/p3H7JD8tdwYpNkBCCtjoxXO7WovNBjdGA H+tIKLWBwL+wj9tjnGQJD6CcjAKu8BgnveK2tCm4qC22sgbIjv/m0JjNyR/deXn1kdIxqq qfq4uKuZvzza7WCmYS/I4JIBjwg1wSWhNccalmi8sH8gzOMOugOK8GwKIxebQg== ARC-Seal: i=1; s=key1; d=yhetil.org; t=1621373168; a=rsa-sha256; cv=none; b=G2yrDCexBnnhOv+HHMRpLmR4YpWne7tSO115cw1pIKKHTRY+p0qBQ4PAfnCH0EG9dezLay lPbAwaRNAD09y4I21Te1f/VOeMNLC0XoxvBitJU9mjkSXMrtXbYgGDwyl21GDqd0A2YR9Y XUevfHAwWMw31cEo8bi/zQ+1sonZLoDnYDguVqu4SgYP+8/YG5UEcyrtKKviu2eMq4tCYP 1wzxOjpcTqfkFYxg/rNLe5KejoIsS/La7sIlu+owMiCkiIrHjMEqNQ8MtIqorXSBy65YM5 E6fsYdZyVSVn/aLMkP21UphddDZTbJXZrNh7WkbLgBYOqF+cWDvdoONVDkNz0Q== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=telenet.be header.s=r21 header.b=LLYhww3R; dmarc=fail reason="SPF not aligned (relaxed)" header.from=telenet.be (policy=none); spf=pass (aspmx1.migadu.com: domain of guix-patches-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=guix-patches-bounces@gnu.org X-Migadu-Spam-Score: -3.44 Authentication-Results: aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=telenet.be header.s=r21 header.b=LLYhww3R; dmarc=fail reason="SPF not aligned (relaxed)" header.from=telenet.be (policy=none); spf=pass (aspmx1.migadu.com: domain of guix-patches-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=guix-patches-bounces@gnu.org X-Migadu-Queue-Id: 7C83B1AE78 X-Spam-Score: -3.44 X-Migadu-Scanner: scn0.migadu.com X-TUID: 8uUd8VbTOt6q --=-JB+lpO7hFMU1O10sr8Bc Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Ludovic Court=C3=A8s schreef op di 18-05-2021 om 22:51 [+0200]: > Hi Maxime, >=20 > Maxime Devos skribis: >=20 > > This is version two of the patch series, removing a 'pk' that > > I added for debugging, and also fixing 'wrap-script' and 'wrap-program'= . > > To fix 'wrap-script' and 'wrap-program', I added a required 'inputs' ar= gument. > > All callers have been adjusted to pass it. > >=20 > > Perhaps 'patch-shebang' can be fixed and needs to be fixed, but I don't > > have investigated that closely yet. ('patch-shebangs' (with a #\s) wor= ks > > properly IIUC). >=20 > Thanks for this long patch series, and sorry for the equally long delay! >=20 > Since we don=E2=80=99t get to change those interfaces very often, I=E2=80= =99m going to > nitpick somewhat because I think we=E2=80=99d rather get them right. >=20 > > From 42e7cf4ca6e4d6e1cd31c2807f608275a5ca759a Mon Sep 17 00:00:00 2001 > > From: Maxime Devos > > Date: Sun, 18 Apr 2021 12:45:13 +0200 > > Subject: [PATCH 1/7] build: Add argument to which for specifying where = to > > search. > > MIME-Version: 1.0 > > Content-Type: text/plain; charset=3DUTF-8 > > Content-Transfer-Encoding: 8bit > >=20 > > The procedure =E2=80=98which=E2=80=99 from (guix build utils) > > is used for two different purposes: > >=20 > > 1. for finding the absolute file name of a binary > > that needs to run during the build process > >=20 > > 2. for finding the absolute file name of a binary, > > for the target system (as in --target=3DTARGET), > > e.g. for substituting sh->/gnu/store/.../bin/sh, > > python->/gnu/store/.../bin/python. >=20 > But note that only #1 is the intended purpose. It is? Then it seems the first patch can be dropped and replaced with something else, as you mentioned below. > > When compiling natively (SYSTEM=3DTARGET modulo nix/autotools differenc= es), > > this is perfectly fine. >=20 > Rather =E2=80=9Ctarget=3D#f=E2=80=9D in Guix parlance Yes, correct. > [...] >=20 > > +(define* (which program #:optional inputs) > > + "Return the complete file name for PROGRAM as found in $PATH, or #fa= lse if > > +PROGRAM could not be found. If INPUTS is not #false, instead look in = the > > +/bin and /sbin subdirectories of INPUTS. INPUTS is an alist; its keys > > +are ignored." >=20 > I find that this leads to a weird interface; =E2=80=98which=E2=80=99 is i= ntended to be > like the same-named shell command, and the notion of =E2=80=9Cinput alist= s=E2=80=9D > seems out of place to me. > > I was thinking we could make it: >=20 > --8<---------------cut here---------------start------------->8--- > (define* (which program #:optional > (path (search-path-as-string->list (getenv "PATH")))) > "Return the complete file name for PROGRAM as found in $PATH, or #f if > PROGRAM could not be found." > (search-path path program)) > --8<---------------cut here---------------end--------------->8--- >=20 > =E2=80=A6 but that doesn=E2=80=99t buy us much. >=20 > I think what we need is to do is find and fix misuses of =E2=80=98which= =E2=80=99. >=20 > WDYT? The current correct way is (string-append (assoc-ref inputs "the-input") "/bin/the-binary") which can easily lead to long lines. Ideally, there would be a shorter way to do this, such as ... the weird interface above. Or the "search-input-file" from below. >=20 > [...] >=20 > > +Here is an example using the @code{which} procedure in a build phase: > > + > > +@lisp > > +(lambda* (#:key outputs inputs #:allow-other-keys) > > + (let ((growpart (string-append (assoc-ref outputs "out") > > + "/bin/growpart"))) > > + (wrap-program growpart > > + `("PATH" ":" prefix (,(dirname (which "sfdisk" inpu= ts)) > > + ,(dirname (which "readlink" in= puts))))))) >=20 > That looks weird to me. The "dirname" & "which" look weird to me to! I grabbed that from some package definition. I guess a different example is needed. > The =E2=80=9Ccorrect=E2=80=9D way we do it right now is like > this: >=20 > (lambda* (#:key outputs inputs #:allow-other-keys) > (let ((out (assoc-ref outputs "out")) > (curl (assoc-ref inputs "curl"))) > (wrap-program (string-append out "/bin/akku") > `("LD_LIBRARY_PATH" ":" prefix (,(string-append curl "/lib"= )))) > #t)) >=20 > Here, when cross-compiling, (assoc-ref inputs "curl") returns the target > cURL. This is something that can be fixed on 'core-updates', right? At least when fixing the package definitions doesn't cause to many rebuilds. > > From e78d2d8651d5f56afa7d57be78c5cccccebb117a Mon Sep 17 00:00:00 2001 > > From: Maxime Devos > > Date: Sun, 18 Apr 2021 20:44:28 +0200 > > Subject: [PATCH 3/7] build: utils: Make inputs of 'wrap-script' explici= t. > >=20 > > Previously, 'wrap-script' used (which "guile") to determine where to lo= cate > > the guile interpreter. But this is incorrect when cross-compiling. Wh= en > > cross-compiling, this would locate the (native) guile interpreter that = is > > in the PATH, while a guile interpreter for the target is required. > >=20 > > Remove the optional #:guile argument which is only used in tests and re= place > > it with a required 'inputs' argument and adjust all callers. Write a n= ew > > test verifying a guile for the target is located, instead of a native g= uile. >=20 > I think the #:guile argument was a fine interface: clear and > to-the-point. The problem IMO is just that it=E2=80=99s not use where it > should. :-) It should be used practically everywhere, no? So making it optional doesn't make much sense to me when we want to support cross-compilation. >=20 > [...] >=20 > > --- a/gnu/packages/audio.scm > > +++ b/gnu/packages/audio.scm > > @@ -4712,9 +4712,9 @@ as is the case with audio plugins.") > > (chmod (string-append out "/share/carla/carla") #o555) > > #t))) > > (add-after 'install 'wrap-executables > > - (lambda* (#:key outputs #:allow-other-keys) > > + (lambda* (#:key inputs outputs #:allow-other-keys) > > (let ((out (assoc-ref outputs "out"))) > > - (wrap-script (string-append out "/bin/carla") > > + (wrap-script (string-append out "/bin/carla") inputs > > `("GUIX_PYTHONPATH" ":" prefix (,(getenv "= GUIX_PYTHONPATH")))) >=20 > This would become: >=20 > (wrap-script (string-append out "/bin/carla") > `(=E2=80=A6) > #:guile (assoc-ref inputs "guile")) > > WDYT? Ok, this looks a good interface to me, though I think 'wrap-script' will need to be modified. IIRC, #:guile must be the full file name of the guile binary and not simply /gnu/store/[...]-guile-$VERSION. > > From 8b843f0dd8803120718747b480983bd5888b1617 Mon Sep 17 00:00:00 2001 > > From: Maxime Devos > > Date: Mon, 19 Apr 2021 16:56:00 +0200 > > Subject: [PATCH 6/7] build: utils: wrap-program: look up bash in inputs= , not > > in PATH > >=20 > > 'wrap-program' is almost always used for creating wrappers for the > > target system. It is only rarely (once) used for creating wrappers for > > the native system. However, 'wrap-program' always creates wrappers for > > the native system and provides no option for creating wrappers for the > > target system instead. >=20 > [...] >=20 > > - (wrap-program > > - (string-append libexec "/dhclient-script") > > + (wrap-program (string-append libexec "/dhclient-scrip= t") > > + inputs > > `("PATH" ":" prefix > > ,(map (lambda (dir) > > (string-append dir "/bin:" >=20 > I=E2=80=99m also skeptical here; =E2=80=98wrap-program=E2=80=99 needs to = know the file name of > =E2=80=98sh=E2=80=99 and instead we=E2=80=99re passing it the full input = list. >=20 > I would instead add #:bash (or #:sh?). The downside is that it=E2=80=99d= be a > bit more verbose, but in terms of interfaces, I=E2=80=99d find it clearer= : >=20 > (wrap-program (string-append libexec "/dhclient-script") > `("PATH" =E2=80=A6) > #:sh (string-append (assoc-ref inputs "bash") "/bin/sh")) LGTM, though rather verbose. > We could introduce a helper procedure to replace (string-append =E2=80=A6= ) with: >=20 > (search-input-file inputs "/bin/sh") > where: >=20 > (define (search-input-file inputs file) > (any (match-lambda > ((label . directory) > (let ((file (string-append directory "/" file))) > (and (file-exists? file) file)))) > inputs)) >=20 > WDYT? That should help with the verbosity. The previous code becomes (wrap-program (string-append libexec "/dhclient-script") `("PATH" =E2=80=A6) #:sh (search-input-file inputs "bin/sh")) which isn't overly verbose. This procedure 'search-input-file' would return #f if the input was not found, right? I would rather it raises an exception instead. There have been some bugs where "#f" was silently written into some file, which is unlikely to work well. For the few cases were the input binary is truly optional, we can define a 'search-optional-input-file' procedure. >=20 >=20 > > + (wrap-program (string-append out "/bin/screenfetch") > > + %build-inputs >=20 > As a rule of thumb we should refer to #:inputs and #:outputs instead of > the global variables =E2=80=98%build-inputs=E2=80=99 etc. Agreed, that's what I thought as well, but that seems like a separate (stylistic) bug to fix. IIRC, the surrounding code used %build-inputs instead of #:inputs. > [...] > > diff --git a/doc/guix.texi b/doc/guix.texi > > index a2ff13fe0f..6235ae9bf7 100644 > > --- a/doc/guix.texi > > +++ b/doc/guix.texi > > @@ -8703,7 +8703,42 @@ Here is an example using the @code{which} proced= ure in a build phase: > > This section documents procedures that create =E2=80=98wrappers=E2=80= =99 around existing > > binaries, that e.g. set environment variables required during executio= n. > > =20 > > -@c TODO document wrap-program > > +@deffn {Scheme Procedure} wrap-program @var{prog} @var{inputs} @var{va= rs} > > +Make a wrapper for @var{prog}. @var{vars} should look like this: > > + > > +@lisp > > + '(VARIABLE DELIMITER POSITION LIST-OF-DIRECTORIES) > ^ > You can remove indentation and use @var instead of capital letters. @var can be used inside @lisp? Didn't know that. > [...] > > This one can even go to master. Yep. Greetings, Maxime. --=-JB+lpO7hFMU1O10sr8Bc Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYKQwyBccbWF4aW1lZGV2 b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7uOPAQCvgzmdOGw7jwwekusHMPS3LrmW Glhc4/xoRIhChy6RSgD/YotwTIWnYiZXVwbWkmCNazJVl8lH1dCCvTEDhTHZSQY= =RELI -----END PGP SIGNATURE----- --=-JB+lpO7hFMU1O10sr8Bc--