GNU bug report logs - #73833
[PATCH] guix: import: composer: Improve composer-fetch.

Previous Next

Full log

View this message in rfc822 format

From: Ludovic Courtès <ludo <at> gnu.org>
To: Nicolas Graves <ngraves <at> ngraves.fr>
Cc: 73833 <at> debbugs.gnu.org
Subject: [bug#73833] [PATCH v2 1/5] guix: import: utils: Add function git->origin.
Date: Wed, 06 Nov 2024 16:16:18 +0100

Hi,

Nice patch series!

Nicolas Graves <ngraves <at> ngraves.fr> skribis:

> * guix/import/utils.scm: (git->origin): Add function.
>
> * guix/import/elpa.scm
> (download-git-repository): Remove function download-git-repository.
> (git-repository->origin): Remove function git-repository->origin.
> (ref): Add function ref.
> (melpa-recipe->origin): Use functions git->origin and ref.
>
> * guix/import/go.scm
> (git-checkout-hash): Remove function git-checkout-hash.
> (transform-version): Add function transform-version.
> (vcs->origin): Use functions git->origin and transform-version. Add
> optional argument transform-version.
>
> * tests/import/go.scm
> (go-module->guix-package): Adapt test case to changes in guix/import/go.scm.
>
> * guix/import/minetest.scm
> (download-git-repository): Remove function download-git-repository.
> (make-minetest-sexp): Use function git->origin.
>
> * tests/minetest.scm
> (make-package-sexp): Use function git->origin.
> (example-package): Adapt test-case to git->origin.
>
> * guix/import/composer.scm
> (make-php-sexp): Use function git->origin.
>
> Change-Id: Ied05a63bdd60fbafe26fbbb4e115ff6f0bb9db3c

[…]

> +(define (ref recipe)
> +  "Create REF from MELPA RECIPE."

More like: "Return a value suitable for the 'update-cached-checkout'
#:ref argument corresponding to RECIPE, a MELPA recipe alist."

> +;; This is done because the version field of the package, which the generated
> +;; quoted expression refers to, has been stripped of any 'v' prefixed.
> +(define (transform-version version)

Please add a docstring.

> -  ;; Use a custom cache to avoid cluttering the default one under
> -  ;; ~/.cache/guix, but choose one under /tmp so that it's persistent across
> -  ;; subsequent "guix import" invocations.
> -  (mkdir-p cache)
> -  (chmod cache #o700)
> -  (let-values (((checkout commit _)
> -                (parameterize ((%repository-cache-directory cache))
> -                  (update-cached-checkout url

Looks like this bit and its rationale in (guix import go) gets lost
here: ‘git->origin’ unconditionally uses ~/.cache, which means that
‘guix import go -r …’ would fill that directory.

Could we restore that behavior, probably as an option to ‘git->origin’?

> +control system is being used. Optionally use the function TRANSFORM-VERSION
> +which takes version as an input."

Two spaces after end-of-sentence period please.  :-)

“Call TRANSFORM-VERSION with VERSION as an argument to compute the
downstream version number.”

You can have #:key (transform-version identity) and call it
unconditionally (that is, it would always be a procedure, never #f).

> +(define* (git->origin repo-url ref #:key (ref->commit #f))
> +  "Returns a generated `origin' block of a package depending on the git source
> +control system, and the directory in the store where the package has been
> +downloaded, in case further processing is necessary.  REPO-URL or REF can be
> +null. REF->COMMIT can be a function or #t, in which case the commit matching
> +ref is used. If REF->COMMIT is not used, the value inside REF is used."

s/function/procedure/

> +  (let* ((version (and (pair? ref) (cdr ref)))
> +         (directory commit
> +                    (if version
> +                        (with-store store
> +                          (latest-repository-commit store repo-url
> +                                                    #:ref (if version ref '())))
> +                        (values #f #f)))
> +         (vcommit (match ref->commit
> +                    (#t    commit)
> +                    (#f    version)
> +                    ((? procedure?) (ref->commit version))
> +                    (_     #f))))

Weird semantics for ‘ref->commit’.  Could it not always be a procedure?

Also, s/vcommit/commit-string/ or something.  See the coding style
regarding identifiers (info "(guix) Formatting Code").

Likewise, could you remove uses of car/cdr in this patch set, at least
for new code (info "(guix) Data Types and Pattern Matching")?

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.