From debbugs-submit-bounces@debbugs.gnu.org Mon Mar 06 14:13:21 2023 Received: (at 61949) by debbugs.gnu.org; 6 Mar 2023 19:13:21 +0000 Received: from localhost ([127.0.0.1]:43642 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pZGGu-00010E-V1 for submit@debbugs.gnu.org; Mon, 06 Mar 2023 14:13:21 -0500 Received: from mail-qv1-f54.google.com ([209.85.219.54]:45683) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pZGGt-000100-1u for 61949@debbugs.gnu.org; Mon, 06 Mar 2023 14:13:19 -0500 Received: by mail-qv1-f54.google.com with SMTP id bo10so7336919qvb.12 for <61949@debbugs.gnu.org>; Mon, 06 Mar 2023 11:13:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1678129993; 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=Dm9U0S6UKrGR28o87r+VPUrx0faCfdOap0VtLk5zwQ4=; b=C99Ll1MvoIwdEzUTe0MwwL5JD5TTe9LTubisoGUQeNhdMv6ClynHZ85JDPuf1BCB8k 2MtYo36pG1QLxE8vgWHf7gHb96XJCf/LZeGYbAekbLY2z6Bz7Oc4O74cnKg8c4SWhx97 Nwlf4sYsT6RuOFQJGpWMAX1kJSvzEEEQ/KXarFyif2pttXHfTsMZgpDsDINLT7pgbEHz SyNqEryHdGEovlCXIQLg4uHBdJEG98NmK1wlLiZm8zLKpwEPGMzSfrZ4yY5xx4ogaOd4 x+7LrjvENIOzHSTKJ9sr6mNOLwwI5lOgSlAsJeGRk1R6/oh9qPop2ZEjCQqL3okSYO6V uMvA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678129993; 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=Dm9U0S6UKrGR28o87r+VPUrx0faCfdOap0VtLk5zwQ4=; b=6PSqRFZWA5vkk0ZG9+I8029smwBv0t7HILJ2CzmZIV4T0UkHMhzk+xoumU0EDM19wO plAQruBh5OXT/0TiwbcszRmRIXnPAr6VUQo3LDsgjj6kb89KpOeTPdFP5QP1Gpxev+o3 SjXC+zK8FjasM/hbwHcWaIkNBTyAOQMIs7TkYHhGDYT0hcIrooMG4fAvCy70ZC9OqlK9 OKp5dBWDVPKojPpTfQ/MlCcdtNyslmZf0kgtRLCzgcxsFQVmJyf8vEcAWAcmLMfPz+oZ ogjpuRJ/cDIXnw5MhiPpO8xMw6eKD3eMxH7tMGI9S7tnOojxDRaYvA09UX0jR+CpDcbu VUrQ== X-Gm-Message-State: AO0yUKVtamnh4LKLy0axnRwb2grAiclzSYRJhMkU9EJMQmKl0W5/kiZ/ 4+JYv3CXRVExz01Fu9Y5H6/XfKza+AU= X-Google-Smtp-Source: AK7set8g1ERs8/ZB+oMxG+aNXgLfRxsZO7/lcQO9w4XwhNaJKxDofdj7Yy2s86um61BGQ8ukSsQC1Q== X-Received: by 2002:ad4:5ca4:0:b0:4c7:595c:9940 with SMTP id q4-20020ad45ca4000000b004c7595c9940mr20879661qvh.51.1678129993113; Mon, 06 Mar 2023 11:13:13 -0800 (PST) Received: from hurd (dsl-149-144.b2b2c.ca. [66.158.149.144]) by smtp.gmail.com with ESMTPSA id t190-20020a3746c7000000b0074235745fdasm7925907qka.58.2023.03.06.11.13.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Mar 2023 11:13:12 -0800 (PST) From: Maxim Cournoyer To: Ludovic =?utf-8?Q?Court=C3=A8s?= Subject: Re: [bug#61949] [PATCH] pack: Move common build code to (guix build pack). References: <20230304031523.24102-1-maxim.cournoyer@gmail.com> <87sfehanlb.fsf@gnu.org> Date: Mon, 06 Mar 2023 14:13:11 -0500 In-Reply-To: <87sfehanlb.fsf@gnu.org> ("Ludovic =?utf-8?Q?Court=C3=A8s=22'?= =?utf-8?Q?s?= message of "Mon, 06 Mar 2023 16:47:28 +0100") Message-ID: <87ttyxzoag.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-Spam-Score: 0.0 (/) X-Debbugs-Envelope-To: 61949 Cc: 61949@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) Hi Ludovic, Ludovic Court=C3=A8s writes: > Hi, > > Maxim Cournoyer skribis: > >> The rationale is to reduce the number of derivations built per pack to i= deally >> one, to minimize storage requirements. The number of derivations had go= ne up >> with 68380db4 ("pack: Extract populate-profile-root from >> self-contained-tarball/builder.") as a side effect to improving code reu= se. >> >> * guix/scripts/pack.scm (guix): Add commentary comment. >> (populate-profile-root, self-contained-tarball/builder): Extract to... >> * guix/build/pack.scm (populate-profile-root!): ... this, and... >> (build-self-contained-tarball): ... that, adjusting for use on the build= side. >> (assert-utf8-locale): New procedure. >> (self-contained-tarball, debian-archive, rpm-archive): Adjust accordingl= y. > > Thanks for working on it! > [...] > >> +;;; Copyright =C2=A9 2021, 2023 Maxim Cournoyer > > This may be inaccurate given that some of the code here predates this > file. > >> ;;; along with GNU Guix. If not, see . >> >> +;;; Commentary: >> + >> +;;; This module contains build-side common procedures used by the host-= side >> +;;; (guix scripts pack) module, mostly to allow for code reuse. Due to= making >> +;;; use of the (guix build store-copy) module, it transitively requires= the >> +;;; sqlite and gcrypt extensions to be available. >> + >> +;;; Code: >> + >> (define-module (guix build pack) > > Commentary/code should come after =E2=80=98define-module=E2=80=99. Eh. I remembered getting it wrong last time, and tried finding the information in the Guile Reference manual; I ended up looking at the "module/scripts/display-commentary.scm" source of the Guile tree, which has: --8<---------------cut here---------------start------------->8--- [...] ;;; Commentary: ;; Usage: display-commentary REF1 REF2 ... ;; ;; Display Commentary section from REF1, REF2 and so on. ;; Each REF may be a filename or module name (list of symbols). ;; In the latter case, a filename is computed by searching `%load-path'. ;;; Code: (define-module (scripts display-commentary) :use-module (ice-9 documentation) :export (display-commentary)) --8<---------------cut here---------------end--------------->8--- Is this wrong? It seems the module implementing the functionality should have gotten that right, ha! Fixed. >> +(define (assert-utf8-locale) >> + "Verify the current process is using the en_US.utf8 locale." >> + (unless (string=3D? "unset for tests" (getenv "GUIX_LOCPATH")) >> + (unless (false-if-exception (setlocale LC_ALL "en_US.utf8")) >> + (error "environment not configured for en_US.utf8 locale")))) >> + >> +(define* (populate-profile-root! profile >> + #:key (profile-name "guix-profile") >> + localstatedir? >> + store-database >> + deduplicate? >> + (symlinks '())) > > Please leave out the bang from the name. The convention in Scheme is to > suffix a name with bang when it modifies the object(s) it=E2=80=99s given; > that=E2=80=99s not the case here (see also =E2=80=98mkdir=E2=80=99, =E2= =80=98open-output-file=E2=80=99, etc.). I see. I wasn't sure, thanks. Fixed. >> + "Populate the root profile directory with SYMLINKS and a Guix databas= e, when >> +LOCALSTATEDIR? is set, and a pre-computed STORE-DATABASE is provided. = The >> +directory is created as \"root\" in the current working directory. When >> +DEDUPLICATE? is true, deduplicate the store items, which relies on hard >> +links. It needs to run in an environment where " >> + (when localstatedir? >> + (unless store-database >> + (error "missing STORE-DATABASE argument"))) >> + >> + (define symlink->directives > > Please move the =E2=80=98when=E2=80=99 expression after all defines so th= at this code > can be interpreted by Guile 2.0, which in turn will allow us to run > tests on =E2=80=98guile-bootstrap=E2=80=99. Done, but there were more complications to get the correct Guile running (because of the new gcrypt extension dependency introduced with the move of 'populate-profile-root' to inside the build module). >> +(define* (build-self-contained-tarball profile >> + tarball-file-name >> + #:key (profile-name "guix-profil= e") >> + target >> + localstatedir? >> + store-database >> + deduplicate? >> + symlinks >> + compressor-command >> + archiver) >> + "Create a self-contained tarball TARBALL-FILE-NAME from PROFILE, opti= onally >> +compressing it with COMPRESSOR-COMMAND, the complete command-line strin= g to >> +use for the compressor." >> + (assert-utf8-locale) >> + >> + (populate-profile-root! profile >> + #:profile-name profile-name >> + #:localstatedir? localstatedir? >> + #:store-database store-database >> + #:deduplicate? deduplicate? >> + #:symlinks symlinks) >> + >> + (define tar (string-append archiver "/bin/tar")) > > Likewise, move defines before statements. Done. > Also, I would just assume =E2=80=9Ctar=E2=80=9D is in $PATH. That=E2=80= =99s the assumption > generally made for things that need to shell out to various commands, > such as (gnu build file-systems), (guix docker), etc. Done. I also dropped the extraneous #:target argument of the 'build-self-contained-tarball' procedure. >> ;;; along with GNU Guix. If not, see . >> >> +;;; Commentary: >> + >> +;;; This module implements the 'guix pack' command and the various supp= orted >> +;;; formats. Where feasible, the builders of the packs should be imple= mented >> +;;; as single derivations to minimize storage requirements. >> + >> +;;; Code: > > Likewise needs to be moved down. :-) Done. >> -(test-assertm "self-contained-tarball" %store >> - (mlet* %store-monad >> - ((profile -> (profile >> - (content (packages->manifest (list %bootstrap-guile= ))) >> - (hooks '()) >> - (locales? #f))) >> - (tarball (self-contained-tarball "pack" profile >> - #:symlinks '(("/bin/Guile" >> - -> "bin/guile")) >> - #:compressor %gzip-compressor >> - #:archiver %tar-bootstrap)) > > [...] > >> ;; The following test needs guile-sqlite3, libgcrypt, etc. as a consequ= ence of >> ;; commit c45477d2a1a651485feede20fe0f3d15aec48b39 and related changes.= Thus, >> ;; run it on the user's store, if it's available, on the grounds that t= hese >> ;; dependencies may be already there, or we can get substitutes or buil= d them >> ;; quite inexpensively; see . >> - >> (with-external-store store >> + (unless store (test-skip 1)) >> + (test-assertm "self-contained-tarball" store > > We should avoid moving this tests here. The goal is to keep as many > tests as possible under the =E2=80=9Cnormal mode=E2=80=9D (outside > =E2=80=98with-external-store=E2=80=99) because they are exercised more fr= equently. I tried avoiding it, but I think it's because of the new gcrypt 'with-extensions' requirement that is now needed for the populate-profile-root that runs on the build side, as explained above. It would attempt to build guile-default and others, like the earlier problem we've had. > I went to great lengths to make it possible here, so we should strive to > preserve that property. I also appreciate the value of being able to run things without a true store/daemon. > (Note that I haven=E2=80=99t tried running the code and tests yet.) > > Could you send a v2? It will follow shortly. By the way, any clue why this happens? --8<---------------cut here---------------start------------->8--- $ make check TESTS=3Dtests/pack.sh [...] PASS: tests/pack.scm --8<---------------cut here---------------end--------------->8--- I'd have expected PASS: tests/pack.sh Thanks! Maxim