[PATCH] ui: Search channels for guix extensions

  • Open
  • quality assurance status badge
Details
3 participants
  • Brian Kubisiak
  • Carlo Zancanaro
  • Ludovic Courtès
Owner
unassigned
Submitted by
Brian Kubisiak
Severity
normal
B
B
Brian Kubisiak wrote 3 days ago
(address . guix-patches@gnu.org)
2fc5afe876af28643f8074dd1623640cb314cc5e.1733064752.git.brian@kubisiak.com
* guix/describe.scm (add-channels-to-load-path!): New function.
* gnu/packages.scm (%package-module-path): Call new function. Remove
the code that the function call replaces.
* guix/ui.scm (extension-directories): Call new function. Search
channels for guix extensions.
* guix/self.scm (compiled-guix)[*core-modules*]: Add 'guile-git' to
the list of extensions.

Change-Id: I53af828dc554485ca28389c9e2653ea6b4fb6b7e
---
gnu/packages.scm | 7 ++++---
guix/describe.scm | 13 +++++++++++++
guix/self.scm | 1 +
guix/ui.scm | 13 ++++++++++---
4 files changed, 28 insertions(+), 6 deletions(-)

Toggle diff (110 lines)
diff --git a/gnu/packages.scm b/gnu/packages.scm
index 80c22d1d7f..05b8bf8e6d 100644
--- a/gnu/packages.scm
+++ b/gnu/packages.scm
@@ -147,15 +147,16 @@ (define %package-module-path
(let* ((not-colon (char-set-complement (char-set #\:)))
(environment (string-tokenize (or (getenv "GUIX_PACKAGE_PATH") "")
not-colon))
- (channels-scm channels-go (package-path-entries)))
+ (channels-scm (package-path-entries)))
;; Automatically add channels and items from $GUIX_PACKAGE_PATH to Guile's
;; search path. For historical reasons, $GUIX_PACKAGE_PATH goes to the
;; front; channels go to the back so that they don't override Guix' own
;; modules.
+ (add-channels-to-load-path!)
(set! %load-path
- (append environment %load-path channels-scm))
+ (append environment %load-path))
(set! %load-compiled-path
- (append environment %load-compiled-path channels-go))
+ (append environment %load-compiled-path))
(make-parameter
(append environment
diff --git a/guix/describe.scm b/guix/describe.scm
index a4ca2462f4..3bbda15db5 100644
--- a/guix/describe.scm
+++ b/guix/describe.scm
@@ -27,6 +27,7 @@ (define-module (guix describe)
sexp->channel
manifest-entry-channel)
#:use-module (srfi srfi-1)
+ #:use-module (srfi srfi-11)
#:use-module (srfi srfi-34)
#:use-module (ice-9 match)
#:export (current-profile
@@ -34,6 +35,7 @@ (define-module (guix describe)
current-profile-entries
current-channels
package-path-entries
+ add-channels-to-load-path!
package-provenance
package-channels
@@ -190,6 +192,17 @@ (define (package-path-entries)
"/site-ccache")))
(current-channel-entries))))
+(define add-channels-to-load-path!
+ (let ((promise
+ (delay
+ (let-values (((channels-scm channels-go) (package-path-entries)))
+ (set! %load-path
+ (append %load-path channels-scm))
+ (set! %load-compiled-path
+ (append %load-compiled-path channels-go))))))
+ (lambda ()
+ (force promise))))
+
(define (package-channels package)
"Return the list of channels providing PACKAGE or an empty list if it could
not be determined."
diff --git a/guix/self.scm b/guix/self.scm
index 2652688c71..28239d53f5 100644
--- a/guix/self.scm
+++ b/guix/self.scm
@@ -882,6 +882,7 @@ (define* (compiled-guix source #:key
,(local-file "../guix/store/schema.sql")))
#:extensions (list guile-gcrypt
+ guile-git ;for (guix git)
guile-json) ;for (guix swh)
#:guile-for-build guile-for-build))
diff --git a/guix/ui.scm b/guix/ui.scm
index eba12c8616..28690b22bc 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -38,6 +38,7 @@
(define-module (guix ui) ;import in user interfaces only
#:use-module (guix i18n)
#:use-module (guix colors)
+ #:use-module (guix describe)
#:use-module (guix diagnostics)
#:use-module (guix gexp)
#:use-module (guix sets)
@@ -2192,9 +2193,15 @@ (define* (command-files #:optional directory)
(define (extension-directories)
"Return the list of directories containing Guix extensions."
- (filter file-exists?
- (parse-path
- (getenv "GUIX_EXTENSIONS_PATH"))))
+ (add-channels-to-load-path!)
+ (let ((channels (package-path-entries)))
+ (filter file-exists?
+ (append
+ (parse-path
+ (getenv "GUIX_EXTENSIONS_PATH"))
+ (map
+ (cut string-append <> "/guix/extensions")
+ channels)))))
(define (commands)
"Return the list of commands, alphabetically sorted."

base-commit: 369d2698b0bfc3726f8e6d232d43d0dda832225f
prerequisite-patch-id: cd0707c90e1d321f3f16f2f861313dd330e9f0b4
--
2.46.0
L
L
Ludovic Courtès wrote 2 days ago
(name . Brian Kubisiak)(address . brian@kubisiak.com)
87frn63sko.fsf@gnu.org
Hello,

Brian Kubisiak <brian@kubisiak.com> skribis:

Toggle quote (10 lines)
> * guix/describe.scm (add-channels-to-load-path!): New function.
> * gnu/packages.scm (%package-module-path): Call new function. Remove
> the code that the function call replaces.
> * guix/ui.scm (extension-directories): Call new function. Search
> channels for guix extensions.
> * guix/self.scm (compiled-guix)[*core-modules*]: Add 'guile-git' to
> the list of extensions.
>
> Change-Id: I53af828dc554485ca28389c9e2653ea6b4fb6b7e

Before looking further, note that Carlo (Cc’d) has been looking into
this as well:


So there’s a real need :-) and perhaps ways both can be merged.

Ludo’.
C
C
Carlo Zancanaro wrote 33 hours ago
(name . Brian Kubisiak)(address . brian@kubisiak.com)(address . 74633@debbugs.gnu.org)
87mshdknrp.fsf@zancanaro.id.au
Hi Brian!

As Ludo mentioned, I've recently sent a patch to solve the same problem,
but mine is a bit different. I think your change is better in most ways
to mine, but I'll comment on some things inline below.

On Sun, Dec 01 2024, Brian Kubisiak wrote:
Toggle quote (15 lines)
> diff --git a/gnu/packages.scm b/gnu/packages.scm
> index 80c22d1d7f..05b8bf8e6d 100644
> --- a/gnu/packages.scm
> +++ b/gnu/packages.scm
> @@ -147,15 +147,16 @@ (define %package-module-path
> (let* ((not-colon (char-set-complement (char-set #\:)))
> (environment (string-tokenize (or (getenv "GUIX_PACKAGE_PATH") "")
> not-colon))
> - (channels-scm channels-go (package-path-entries)))
> + (channels-scm (package-path-entries)))
> ;; Automatically add channels and items from $GUIX_PACKAGE_PATH to Guile's
> ;; search path. For historical reasons, $GUIX_PACKAGE_PATH goes to the
> ;; front; channels go to the back so that they don't override Guix' own
> ;; modules.

This comment should be moved onto add-channels-to-load-path!. Possibly
even as a docstring.

Toggle quote (19 lines)
> diff --git a/guix/describe.scm b/guix/describe.scm
> index a4ca2462f4..3bbda15db5 100644
> --- a/guix/describe.scm
> +++ b/guix/describe.scm
> @@ -190,6 +192,17 @@ (define (package-path-entries)
> "/site-ccache")))
> (current-channel-entries))))
>
> +(define add-channels-to-load-path!
> + (let ((promise
> + (delay
> + (let-values (((channels-scm channels-go) (package-path-entries)))
> + (set! %load-path
> + (append %load-path channels-scm))
> + (set! %load-compiled-path
> + (append %load-compiled-path channels-go))))))
> + (lambda ()
> + (force promise))))

I like that this is avoiding adding things to the path multiple times
(which is a problem with my proposed change).

Toggle quote (10 lines)
> diff --git a/guix/self.scm b/guix/self.scm
> index 2652688c71..28239d53f5 100644
> --- a/guix/self.scm
> +++ b/guix/self.scm
> @@ -882,6 +882,7 @@ (define* (compiled-guix source #:key
> ,(local-file "../guix/store/schema.sql")))
>
> #:extensions (list guile-gcrypt
> + guile-git ;for (guix git)

I don't know enough to know if this is a problem, but it's a shame to
have to add this, given I don't think this change actually ends up using
any git functionality.

I solved the same issue by lazily resolving the (guix describe) module.
Using an autoload wasn't enough (i.e. it still broke "guix pull"), but
using (@ (guix describe) package-path-entries) worked.

Toggle quote (21 lines)
> diff --git a/guix/ui.scm b/guix/ui.scm
> index eba12c8616..28690b22bc 100644
> --- a/guix/ui.scm
> +++ b/guix/ui.scm
> @@ -2192,9 +2193,15 @@ (define* (command-files #:optional directory)
>
> (define (extension-directories)
> "Return the list of directories containing Guix extensions."
> - (filter file-exists?
> - (parse-path
> - (getenv "GUIX_EXTENSIONS_PATH"))))
> + (add-channels-to-load-path!)
> + (let ((channels (package-path-entries)))
> + (filter file-exists?
> + (append
> + (parse-path
> + (getenv "GUIX_EXTENSIONS_PATH"))
> + (map
> + (cut string-append <> "/guix/extensions")
> + channels)))))

I don't think you need the (append ...). According to the manual,
parse-path takes another argument as a "tail" for the resulting list, so
(parse-path (getenv "GUIX_EXTENSIONS_PATH") (map ... channels)) should
be enough here.

Am I right in thinking that this will look inside the channels at
/guix/extensions to try to find things? That is, if I wanted to add a
command "guix foo" I would need to define a module in
$channel_dir/guix/extensions/guix/scripts/foo.scm?

If I'm reading that correctly, this feels unnecessary. Channels can
already specify a directory in their .guix-channel file. In my change it
just looks for $channel_dir/guix/scripts/foo.scm, which seems more
straightforward. (I'm not worried about people accidentally defining
something /guix/scripts/, because the name is pretty obvious.)

All together, I'm proposing simplifying things to:

Toggle snippet (8 lines)
(define (extension-directories)
"Return the list of directories containing Guix extensions."
(add-channels-to-load-path!)
(let ((channels (package-path-entries)))
(filter file-exists?
(parse-path (getenv "GUIX_EXTENSIONS_PATH") channels))))

We may also want to something into the docstring to mention that it adds
to %load-path.

Carlo
B
B
Brian Kubisiak wrote 20 hours ago
(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)(address . 74633@debbugs.gnu.org)
pfuvhwyvbfi2n3higerccakdnylsv7vbadwjjnzay5sejiot3q@ki6cc4wuwcru
Hi Carlo,

Toggle quote (19 lines)
> On Sun, Dec 01 2024, Brian Kubisiak wrote:
> > diff --git a/gnu/packages.scm b/gnu/packages.scm
> > index 80c22d1d7f..05b8bf8e6d 100644
> > --- a/gnu/packages.scm
> > +++ b/gnu/packages.scm
> > @@ -147,15 +147,16 @@ (define %package-module-path
> > (let* ((not-colon (char-set-complement (char-set #\:)))
> > (environment (string-tokenize (or (getenv "GUIX_PACKAGE_PATH") "")
> > not-colon))
> > - (channels-scm channels-go (package-path-entries)))
> > + (channels-scm (package-path-entries)))
> > ;; Automatically add channels and items from $GUIX_PACKAGE_PATH to Guile's
> > ;; search path. For historical reasons, $GUIX_PACKAGE_PATH goes to the
> > ;; front; channels go to the back so that they don't override Guix' own
> > ;; modules.
>
> This comment should be moved onto add-channels-to-load-path!. Possibly
> even as a docstring.

Will do.

Toggle quote (14 lines)
> > diff --git a/guix/self.scm b/guix/self.scm
> > index 2652688c71..28239d53f5 100644
> > --- a/guix/self.scm
> > +++ b/guix/self.scm
> > @@ -882,6 +882,7 @@ (define* (compiled-guix source #:key
> > ,(local-file "../guix/store/schema.sql")))
> >
> > #:extensions (list guile-gcrypt
> > + guile-git ;for (guix git)
>
> I don't know enough to know if this is a problem, but it's a shame to
> have to add this, given I don't think this change actually ends up using
> any git functionality.

Honestly I'm not sure what the are the downsides of this, since I
don't see how *core-module* would get used without *extra-modules* or
why *core-modules* doesn't include all the dependencies. Maybe Ludo
knows more?

Toggle quote (26 lines)
> > diff --git a/guix/ui.scm b/guix/ui.scm
> > index eba12c8616..28690b22bc 100644
> > --- a/guix/ui.scm
> > +++ b/guix/ui.scm
> > @@ -2192,9 +2193,15 @@ (define* (command-files #:optional directory)
> >
> > (define (extension-directories)
> > "Return the list of directories containing Guix extensions."
> > - (filter file-exists?
> > - (parse-path
> > - (getenv "GUIX_EXTENSIONS_PATH"))))
> > + (add-channels-to-load-path!)
> > + (let ((channels (package-path-entries)))
> > + (filter file-exists?
> > + (append
> > + (parse-path
> > + (getenv "GUIX_EXTENSIONS_PATH"))
> > + (map
> > + (cut string-append <> "/guix/extensions")
> > + channels)))))
>
> I don't think you need the (append ...). According to the manual,
> parse-path takes another argument as a "tail" for the resulting list, so
> (parse-path (getenv "GUIX_EXTENSIONS_PATH") (map ... channels)) should
> be enough here.

I saw that on your patch, good to know!

Toggle quote (3 lines)
> Am I right in thinking that this will look inside the channels at
> /guix/extensions to try to find things?

Correct. I chose this to match existing behavior, since
$GUIX_EXTENSIONS_PATH points at this directory for an extension
installed as a package. (I was able to tkae an existing extension and
pull it in as a channel and everything "just worked" without changing
any code or directory structure in the extension)

Toggle quote (3 lines)
> That is, if I wanted to add a command "guix foo" I would need to
> define a module in $channel_dir/guix/extensions/guix/scripts/foo.scm?

This is not required, the command "guix foo" could be defined in
$channel_dir/guix/extensions/foo.scm. The command-files function
doesn't add /guix/scripts if a directory is passed as an argument and
the run-guix-command function will resolve '(guix extensions foo)
if foo.scm exists in an extension directory
(i.e. $channel_dir/guix/extensions/foo.scm exists).

Thanks for the feedback,
Brian
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 74633@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 74633
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch