[bug#70494] [PATCH 04/23] guix: store: environment: New module.

["Ludovic Courtès" <ludo@gnu.org>]

Hi Chris,

Christopher Baines <mail@cbaines.net> skribis:

> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
>
> * guix/store/environment.scm: New file.
> * guix/store.scm: Export compressed-hash.
> * guix/store/database.scm (output-path-id-sql, outputs-exist?, references-sql,
> file-closure, all-input-output-paths, all-transitive-inputs): New variables.
> (outputs-exist?, file-closure, all-transitive-inputs): Export procedures.
> * Makefile.am (STORE_MODULES): Add guix/store/environment.scm.
>
> Co-authored-by: Christopher Baines <mail@cbaines.net>
> Change-Id: I71ac38fa8596a0c05b34880ca60e8a27ef3892d8

Very cool.  Some comments:

> +++ b/guix/store.scm
> @@ -192,6 +192,7 @@ (define-module (guix store)
>              grafting?
>  
>              %store-prefix
> +            compressed-hash
>              store-path
>              output-path
>              fixed-output-path

We can keep it this way for now.

However, the suggestion I made to reepca back then was that we should
move the low-level hashing/file name computation procedures to a
separate module, say (guix store file-names), such that daemon code does
not import (guix store).

(guix store) would only contain client-side code, possibly re-exporting
some of (guix store file-names) for compatibility and convenience.

> +(define* (file-closure db path #:key (list-so-far vlist-null))
> +  "Return a vlist containing the store paths referenced by PATH, the store
> +paths referenced by those paths, and so on."

s/file-closure/store-item-closure/ ?

> +(define (all-input-output-paths drv)
> +  "Return a list containing the output paths this derivation's inputs need to
> +provide."
> +  (apply append (map derivation-input-output-paths

Use ‘append-map’ instead.

> +  #:export (<environment>

Don’t export record type descriptors in general as this exposes the ABI.

> +            environment-namespaces
> +            environment-variables
> +            environment-temp-dirs

s/temp-dirs/temporary-directories/

> +            environment-filesystems
> +            environment-new-session?
> +            environment-new-pgroup?
> +            environment-setup-i/o-proc
> +            environment-preserved-fds
> +            environment-chroot
> +            environment-personality
> +            environment-user
> +            environment-group
> +            environment-hostname
> +            environment-domainname

I’d write “file-systems”, “host-name”, and “domain-name”, to be
consistent with the rest of the code base (we can keep “namespaces”
because that’s how Linux spells it.)

> +            build-environment-vars

s/vars/variables/

> +(define-record-type* <environment> environment

We should keep in mind that maybe we’ll want to use that in ‘guix shell
-C’ eventually.

> +(define (delete-environment env)
> +  "Delete all temporary directories used in ENV."

s/delete-environment/delete-temporary-directories/

> +(define* (temp-directory tmpdir name #:optional permissions user group)
> +  "Create a temporary directory under TMPDIR with permissions PERMISSIONS if
> +specified, otherwise default permissions as specified by umask, and belonging
> +to user USER and group GROUP (defaulting to current user if not specified or
> +#f).  Return the full filename of the form <tmpdir>/<name>-<number>."

s/temp-directory/create-temporary-directory/

This procedure missed the fix in commit
ec7fb669945bfb47c5e1fdf7de3a5d07f7002ccf (CVE-2021-27851).  It’s fine to
implement it later but we should at least leave a big FIXME comment.

Somewhere we’ll also need the fix for CVE-2024-27297 (commits
ff1251de0bc327ec478fc66a562430fbf35aef42 and
8f4ffb3fae133bb21d7991e97c2f19a7108b1143).

> +(define* (dump-port port #:optional (target-port (current-output-port)))

Use the one from (guix build utils) instead.

Thanks,
Ludo’.