Subject: Re: [bug#70494] [PATCH 05/23] store: build-derivations: New module.

From: Ludovic Courtès <[email-masked]>
Date: Mon, 13 May 2024 17:22:29 +0200

Christopher Baines <[email-masked]> skribis:

> From: Caleb Ristvedt <[email-masked]>
>
> * guix/store/build-derivations.scm (get-output-specs, builtin-download,
> add-to-trie, make-search-trie, remove-from-trie!, scanning-wrapper-port,
> scan-for-references, ensure-input-outputs-exist, build-derivation): New
> procedures.
> (builtins): New variable.
> (<trie-node>): New record types.
> * Makefile.am (STORE_MODULES): Add it.
>
> Co-authored-by: Christopher Baines <[email-masked]>
> Change-Id: I904b75e3c58c5fb996c0c9d1ca19b2cb2beb90b6

I think you can add a copyright line for you.

[...]

> +(define-module (guix store build-derivations)

Or just (guix store build)?

> +  #:use-module (guix derivations)

As for (guix store), we’ll have to fix it eventually: daemon code should
not import client code.

So I would move the <derivation> record and low-level code to (guix
store derivations).  (guix derivations) would import that module.
Anyway, we’ll discuss that separately.

> +  #:use-module (guix build store-copy)
> +  #:use-module (gnu system file-systems)

It’s a good idea to #:autoload as much as possible, especially (gnu …)
modules that may pull lots of other modules.

It would be good to have tests here: maybe low-level unit tests for the
reference scanner (it’s a fairly critical piece :-)), as well as
integration tests that perform a build or something.

Again maybe that can come later if it’s more convenient.

Ludo’.