From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp12.migadu.com ([2001:41d0:306:2d92::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms9.migadu.com with LMTPS id eKwBOSk1C2UV7AAAauVa8A:P1 (envelope-from ) for ; Wed, 20 Sep 2023 20:08:42 +0200 Received: from aspmx1.migadu.com ([2001:41d0:306:2d92::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp12.migadu.com with LMTPS id eKwBOSk1C2UV7AAAauVa8A (envelope-from ) for ; Wed, 20 Sep 2023 20:08:41 +0200 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 91A0A3CA57 for ; Wed, 20 Sep 2023 20:08:41 +0200 (CEST) Authentication-Results: aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=gmail.com header.s=20230601 header.b=MKuqvBZZ; 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"; dmarc=fail reason="SPF not aligned (relaxed)" header.from=gmail.com (policy=none) ARC-Seal: i=1; s=key1; d=yhetil.org; t=1695233321; a=rsa-sha256; cv=none; b=ttXduUJEQ/t6yfR32UhRVy3SjiIrjVGbACXV9GDc7sF8OD88YJnAvcjzOsMr+MvC+A6kSs C6iUCHN2DYhGs9zKdxmfM8IG9Pl4Q53D47uuX74NLn86mMEV+pB/NVvnSJVJz0/u/KB3X8 zwon+3XiUweqGwZCAk+6tyTS+nriUzUcn8dzfZXTO6uBqFAlFCmyAiZT+X7Aby2OEZGwrK KpuZ1UpmS72z0es6hLjVj573N9/boX7uTTLh6iuiDmveBX9/BuMT3MfjB7dv/NoWJvK+jM AQw//CVBmb2frzd17J5QA52/rC8j3+nnZ6chgX161y7jI2/MN3k6/MHlaMTolA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1695233321; 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=58DlJNOwOK3ij5mCkfPhsbgAUOv4gXR0C+7hSWmKqKI=; b=EpZoY6cLqsT0468gHLvcw3yxyTLudFgdD7i+jk56azNRvpSqEsv2gZ5AMWU1DnO0/8Dmfr 4RN+F3REBC6b9qqPOmmDgSgs16w0gdwgCKITj9YJnFflRfgWslpbyFxAuo3wclfiaC3CWs P9m95Q/tWDQuG25lzviSBSHC/31eAnD8LXqn7yjYXStvymy5VAkzLV/DbN1XyJC26mitHj i9B39i2NY+N165NpkjROwuGV2/uib+Mmg71K9OE61RhC1i/0bMlEEpNhs+szHTnAgJGkVk 2IBq+DQldGjleJdMW8FUf8jp/W4q/sQPig9z2/iOXxMrRgqXZlEL0hNVrjVdYw== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=gmail.com header.s=20230601 header.b=MKuqvBZZ; 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"; dmarc=fail reason="SPF not aligned (relaxed)" header.from=gmail.com (policy=none) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qj14P-0002ev-3A; Wed, 20 Sep 2023 13:33:01 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qj14I-0002Zr-1P for guix-patches@gnu.org; Wed, 20 Sep 2023 13:32:54 -0400 Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qj14H-00013Q-AP for guix-patches@gnu.org; Wed, 20 Sep 2023 13:32:53 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qj14Q-0005kG-DV for guix-patches@gnu.org; Wed, 20 Sep 2023 13:33:02 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#65866] [PATCH 0/8] Add built-in builder for Git checkouts Resent-From: Maxim Cournoyer Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Wed, 20 Sep 2023 17:33:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 65866 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: Ludovic =?UTF-8?Q?Court=C3=A8s?= Cc: Josselin Poiret , Simon Tournier , Mathieu Othacehe , Tobias Geerinckx-Rice , Ludovic =?UTF-8?Q?Court=C3=A8s?= , Ricardo Wurmus , 65866@debbugs.gnu.org, Christopher Baines Received: via spool by 65866-submit@debbugs.gnu.org id=B65866.169523117922075 (code B ref 65866); Wed, 20 Sep 2023 17:33:02 +0000 Received: (at 65866) by debbugs.gnu.org; 20 Sep 2023 17:32:59 +0000 Received: from localhost ([127.0.0.1]:60306 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qj14M-0005jy-FW for submit@debbugs.gnu.org; Wed, 20 Sep 2023 13:32:59 -0400 Received: from mail-qv1-xf29.google.com ([2607:f8b0:4864:20::f29]:54696) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qj14J-0005ji-8e for 65866@debbugs.gnu.org; Wed, 20 Sep 2023 13:32:56 -0400 Received: by mail-qv1-xf29.google.com with SMTP id 6a1803df08f44-6563bca1b38so235336d6.1 for <65866@debbugs.gnu.org>; Wed, 20 Sep 2023 10:32:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695231160; x=1695835960; darn=debbugs.gnu.org; h=content-transfer-encoding:mime-version:user-agent:message-id :in-reply-to:date:references:subject:cc:to:from:from:to:cc:subject :date:message-id:reply-to; bh=58DlJNOwOK3ij5mCkfPhsbgAUOv4gXR0C+7hSWmKqKI=; b=MKuqvBZZt/rxLMLM6YhTJ3KqBDLD4yUIjeNrKoNenTXdn0cPphR0RBQRIoSrVxP/pW XOuJaCZKB1gkkJp5LeTOPj1UWuVhCvWK5N/CxCQKgAqvnZAdfYPmXsL+8sMedQ95Q92C BAH01HbPsQEB0jZviPcwkgfRgJs/yJQyB6tz/PqUdTGZUwPRfHd/4spX+GgBijvAMQkL 1IW4lLSdtv+fZUA8yxMuDxOWqw+KMWMKuMNZFgzIfvPHSkMuFdNYToaL16YPiF0U66Td HiR0lqswlsp8FiFSjMNJnXGof7pz2uBcHLCgjxtM3KCPxLKprybhQaIb8hXynLTX5PDH cJ+Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695231160; x=1695835960; h=content-transfer-encoding:mime-version:user-agent:message-id :in-reply-to:date:references:subject:cc:to:from:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=58DlJNOwOK3ij5mCkfPhsbgAUOv4gXR0C+7hSWmKqKI=; b=gVLNdkCULCMQUFlRCs7qRVsAXIaNsfbxunM0Z8IRYYcH+w3lxwbDNpXprhwNxlU7BK 9C64lILrDPWnvQN0lO8RvRV25Pm/VG8vlgTE4ZYVHPKo3XR9g31/Yel0g9hB2+3pmuiX xPBmSg2+1HLIF37JUcWkQ9cucMwiExa1N32Pf6RuQreeVlG7jra0HSk6QmzNRe93G4VB hYP7rrZa8qcE7vG/K9jnSYZxDH5BrWJEp6gNc2Jv1eF9SnA/lxAO1RVwGqt8Kt2N8+bT V+PbSZaDlWb/9b/Ay7m6sWx2ctuzVDWKW71p1WtRL6Rtawe8/CnzgXptJG0rIxctEbRz h9kQ== X-Gm-Message-State: AOJu0YxmJW7wZCxxvE6bVx+09O/erKthnEIV1q38DbLW3+oEGi8WO5nc ++HhmFXmR32XdrQrdqetOXw= X-Google-Smtp-Source: AGHT+IGdqtDnGEt3rSomdn5BZv1kyDXl8iXXKmYGYtfZ3xujLozpPWxj9tCpNufVHd8L6UR0Cs9CwQ== X-Received: by 2002:a05:6214:ab1:b0:651:69d7:3d6a with SMTP id ew17-20020a0562140ab100b0065169d73d6amr2488553qvb.15.1695231160036; Wed, 20 Sep 2023 10:32:40 -0700 (PDT) Received: from hurd (dsl-10-134-200.b2b2c.ca. [72.10.134.200]) by smtp.gmail.com with ESMTPSA id r11-20020a0ce28b000000b00646e0411e8csm3788061qvl.30.2023.09.20.10.32.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 20 Sep 2023 10:32:39 -0700 (PDT) From: Maxim Cournoyer References: <3c42634cb47dd7eaa81a198bc2d097ca74a973ed.1694441831.git.ludo@gnu.org> Date: Wed, 20 Sep 2023 13:32:37 -0400 In-Reply-To: <3c42634cb47dd7eaa81a198bc2d097ca74a973ed.1694441831.git.ludo@gnu.org> ("Ludovic =?UTF-8?Q?Court=C3=A8s?="'s message of "Mon, 11 Sep 2023 16:25:22 +0200") Message-ID: <8734z8698q.fsf_-_@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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-bounces+larch=yhetil.org@gnu.org X-Migadu-Flow: FLOW_IN X-Migadu-Country: US X-Migadu-Spam-Score: -3.60 X-Spam-Score: -3.60 X-Migadu-Queue-Id: 91A0A3CA57 X-Migadu-Scanner: mx2.migadu.com X-TUID: fHIblWQbC63/ Hello, Ludovic Court=C3=A8s writes: > From: Ludovic Court=C3=A8s > > The new builder makes it possible to break cycles that occurs when the > fixed-output derivation for the source of a dependency of =E2=80=98git=E2= =80=99 would > itself depend on =E2=80=98git=E2=80=99. > > * guix/scripts/perform-download.scm (perform-git-download): New > procedure. > (perform-download): Move fixed-output derivation check to=E2=80=A6 > (guix-perform-download): =E2=80=A6 here. Invoke =E2=80=98perform-downloa= d=E2=80=99 or > =E2=80=98perform-git-download=E2=80=99 depending on what =E2=80=98derivat= ion-builder=E2=80=99 returns. > * nix/libstore/builtins.cc (builtins): Add =E2=80=9Cgit-download=E2=80=9D. > * tests/derivations.scm ("built-in-builders"): Update. > ("'git-download' built-in builder") > ("'git-download' built-in builder, invalid hash") > ("'git-download' built-in builder, invalid commit") > ("'git-download' built-in builder, not found"): New tests. > --- > guix/scripts/perform-download.scm | 52 +++++++++++++--- > nix/libstore/builtins.cc | 5 +- > tests/derivations.scm | 100 +++++++++++++++++++++++++++++- > 3 files changed, 145 insertions(+), 12 deletions(-) > > diff --git a/guix/scripts/perform-download.scm b/guix/scripts/perform-dow= nload.scm > index c8f044e82e..a287e97528 100644 > --- a/guix/scripts/perform-download.scm > +++ b/guix/scripts/perform-download.scm > @@ -1,5 +1,5 @@ > ;;; GNU Guix --- Functional package management for GNU > -;;; Copyright =C2=A9 2016, 2017, 2018, 2020 Ludovic Court=C3=A8s > +;;; Copyright =C2=A9 2016-2018, 2020, 2023 Ludovic Court=C3=A8s > ;;; > ;;; This file is part of GNU Guix. > ;;; > @@ -21,7 +21,8 @@ (define-module (guix scripts perform-download) > #:use-module (guix scripts) > #:use-module (guix derivations) > #:use-module ((guix store) #:select (derivation-path? store-path?)) > - #:use-module (guix build download) > + #:autoload (guix build download) (url-fetch) > + #:autoload (guix build git) (git-fetch-with-fallback) > #:use-module (ice-9 match) > #:export (guix-perform-download)) >=20=20 > @@ -64,10 +65,6 @@ (define* (perform-download drv #:optional output > (drv-output (assoc-ref (derivation-outputs drv) "out")) > (algo (derivation-output-hash-algo drv-output)) > (hash (derivation-output-hash drv-output))) > - (unless (and algo hash) > - (leave (G_ "~a is not a fixed-output derivation~%") > - (derivation-file-name drv))) > - > ;; We're invoked by the daemon, which gives us write access to OUT= PUT. > (when (url-fetch url output > #:print-build-trace? print-build-trace? > @@ -92,6 +89,33 @@ (define* (perform-download drv #:optional output > (when (and executable (string=3D? executable "1")) > (chmod output #o755)))))) >=20=20 > +(define* (perform-git-download drv #:optional output > + #:key print-build-trace?) > + "Perform the download described by DRV, a fixed-output derivation, to > +OUTPUT. > + > +Note: Unless OUTPUT is #f, we don't read the value of 'out' in DRV since= the > +actual output is different from that when we're doing a 'bmCheck' or I'd drop the 'we's and use impersonal imperative tense or at least 's/when we're doing/when doing/'. > +'bmRepair' build." > + (derivation-let drv ((output* "out") I'd name this variable just 'out', for consistency with the others. > + (url "url") > + (commit "commit") > + (recursive? "recursive?")) > + (unless url > + (leave (G_ "~a: missing Git URL~%") (derivation-file-name drv))) > + (unless commit > + (leave (G_ "~a: missing Git commit~%") (derivation-file-name drv))) > + > + (let* ((output (or output output*)) >=20 > + (url (call-with-input-string url read)) > + (recursive? (and recursive? > + (call-with-input-string recursive? read))) > + (drv-output (assoc-ref (derivation-outputs drv) "out")) > + (algo (derivation-output-hash-algo drv-output)) > + (hash (derivation-output-hash drv-output))) > + (git-fetch-with-fallback url commit output > + #:recursive? recursive?)))) > + > (define (assert-low-privileges) > (when (zero? (getuid)) > (leave (G_ "refusing to run with elevated privileges (UID ~a)~%") > @@ -120,8 +144,20 @@ (define-command (guix-perform-download . args) > (match args > (((? derivation-path? drv) (? store-path? output)) > (assert-low-privileges) > - (let ((drv (read-derivation-from-file drv))) > - (perform-download drv output #:print-build-trace? print-build-t= race?))) > + (let* ((drv (read-derivation-from-file drv)) > + (download (match (derivation-builder drv) > + ("builtin:download" perform-download) > + ("builtin:git-download" perform-git-download) > + (unknown (leave (G_ "~a: unknown builtin build= er") > + unknown)))) > + (drv-output (assoc-ref (derivation-outputs drv) "out")) > + (algo (derivation-output-hash-algo drv-output)) > + (hash (derivation-output-hash drv-output))) > + (unless (and hash algo) > + (leave (G_ "~a is not a fixed-output derivation~%") > + (derivation-file-name drv))) > + > + (download drv output #:print-build-trace? print-build-trace?))) > (("--version") > (show-version-and-exit)) > (x > diff --git a/nix/libstore/builtins.cc b/nix/libstore/builtins.cc > index 4111ac4760..6bf467354a 100644 > --- a/nix/libstore/builtins.cc > +++ b/nix/libstore/builtins.cc > @@ -1,5 +1,5 @@ > /* GNU Guix --- Functional package management for GNU > - Copyright (C) 2016, 2017, 2018, 2019 Ludovic Court=C3=A8s > + Copyright (C) 2016-2019, 2023 Ludovic Court=C3=A8s >=20=20 > This file is part of GNU Guix. >=20=20 > @@ -58,7 +58,8 @@ static void builtinDownload(const Derivation &drv, >=20=20 > static const std::map builtins =3D > { > - { "download", builtinDownload } > + { "download", builtinDownload }, > + { "git-download", builtinDownload } > }; >=20=20 > derivationBuilder lookupBuiltinBuilder(const std::string & name) > diff --git a/tests/derivations.scm b/tests/derivations.scm > index 66c777cfe7..e1312bd46b 100644 > --- a/tests/derivations.scm > +++ b/tests/derivations.scm > @@ -24,10 +24,15 @@ (define-module (test-derivations) > #:use-module (guix utils) > #:use-module ((gcrypt hash) #:prefix gcrypt:) > #:use-module (guix base32) > + #:use-module ((guix git) #:select (with-repository)) > #:use-module (guix tests) > + #:use-module (guix tests git) > #:use-module (guix tests http) > #:use-module ((guix packages) #:select (package-derivation base32)) > - #:use-module ((guix build utils) #:select (executable-file?)) > + #:use-module ((guix build utils) #:select (executable-file? which)) > + #:use-module ((guix hash) #:select (file-hash*)) > + #:use-module ((git oid) #:select (oid->string)) > + #:use-module ((git reference) #:select (reference-name->oid)) > #:use-module (gnu packages bootstrap) > #:use-module ((gnu packages guile) #:select (guile-1.8)) > #:use-module (srfi srfi-1) > @@ -195,7 +200,7 @@ (define* (directory-contents dir #:optional (slurp ge= t-bytevector-all)) > (stat:ino (lstat file2)))))))) >=20=20 > (test-equal "built-in-builders" > - '("download") > + '("download" "git-download") > (built-in-builders %store)) >=20=20 > (test-assert "unknown built-in builder" > @@ -290,6 +295,97 @@ (define* (directory-contents dir #:optional (slurp g= et-bytevector-all)) > get-string-all) > text)))))) >=20=20 > +;; 'with-temporary-git-repository' relies on the 'git' command. > +(unless (which (git-command)) (test-skip 1)) I'd expect the 'git' command to now be required by Autoconf at build time, which should mean checking it here is not useful/required? > +(test-equal "'git-download' built-in builder" > + `(("/a.txt" . "AAA") > + ("/b.scm" . "#t")) > + (let ((nonce (random-text))) > + (with-temporary-git-repository directory > + `((add "a.txt" "AAA") > + (add "b.scm" "#t") > + (commit ,nonce)) > + (let* ((commit (with-repository directory repository > + (oid->string > + (reference-name->oid repository "HEAD")))) > + (drv (derivation %store "git-download" > + "builtin:git-download" '() > + #:env-vars > + `(("url" > + . ,(object->string > + (string-append "file://" directory)= )) > + ("commit" . ,commit)) > + #:hash-algo 'sha256 > + #:hash (file-hash* directory > + #:algorithm > + (gcrypt:hash-algorithm > + gcrypt:sha256) > + #:recursive? #t) > + #:recursive? #t))) > + (build-derivations %store (list drv)) > + (directory-contents (derivation->output-path drv) get-string-all= ))))) > + > +(unless (which (git-command)) (test-skip 1)) > +(test-assert "'git-download' built-in builder, invalid hash" > + (with-temporary-git-repository directory > + `((add "a.txt" "AAA") > + (add "b.scm" "#t") > + (commit "Commit!")) > + (let* ((commit (with-repository directory repository > + (oid->string > + (reference-name->oid repository "HEAD")))) > + (drv (derivation %store "git-download" > + "builtin:git-download" '() > + #:env-vars > + `(("url" > + . ,(object->string > + (string-append "file://" directory))) > + ("commit" . ,commit)) > + #:hash-algo 'sha256 > + #:hash (gcrypt:sha256 #vu8()) > + #:recursive? #t))) > + (guard (c ((store-protocol-error? c) > + (string-contains (store-protocol-error-message c) "fail= ed"))) > + (build-derivations %store (list drv)) > + #f)))) > + > +(unless (which (git-command)) (test-skip 1)) > +(test-assert "'git-download' built-in builder, invalid commit" > + (with-temporary-git-repository directory > + `((add "a.txt" "AAA") > + (add "b.scm" "#t") > + (commit "Commit!")) > + (let* ((drv (derivation %store "git-download" > + "builtin:git-download" '() > + #:env-vars > + `(("url" > + . ,(object->string > + (string-append "file://" directory))) > + ("commit" > + . "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa= aa")) > + #:hash-algo 'sha256 > + #:hash (gcrypt:sha256 #vu8()) > + #:recursive? #t))) > + (guard (c ((store-protocol-error? c) > + (string-contains (store-protocol-error-message c) "fail= ed"))) > + (build-derivations %store (list drv)) > + #f)))) > + > +(test-assert "'git-download' built-in builder, not found" > + (let* ((drv (derivation %store "git-download" > + "builtin:git-download" '() > + #:env-vars > + `(("url" . "file:///does-not-exist.git") > + ("commit" > + . "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa= ")) > + #:hash-algo 'sha256 > + #:hash (gcrypt:sha256 #vu8()) > + #:recursive? #t))) > + (guard (c ((store-protocol-error? c) > + (string-contains (store-protocol-error-message c) "failed= "))) > + (build-derivations %store (list drv)) > + #f))) > + Maybe the error message compared could be more precised, if it already contains the necessary details? Otherwise, well done! LGTM with my above comments. --=20 Thanks, Maxim