[PATCH] ui: Search channels for guix extensions

  • Done
  • quality assurance status badge
Details
4 participants
  • Brian Kubisiak
  • Carlo Zancanaro
  • Carlo Zancanaro
  • Ludovic Courtès
Owner
unassigned
Submitted by
Brian Kubisiak
Severity
normal
Merged with

Debbugs page

Brian Kubisiak wrote 3 months 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
Ludovic Courtès wrote 3 months ago
(name . Brian Kubisiak)(address . brian@kubisiak.com)(address . 74633@debbugs.gnu.org)(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)
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’.
Carlo Zancanaro wrote 3 months 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
Brian Kubisiak wrote 3 months 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
Carlo Zancanaro wrote 3 months ago
(name . Brian Kubisiak)(address . brian@kubisiak.com)(address . 74633@debbugs.gnu.org)
87cyi7q8iu.fsf@sil.org
On Tue, Dec 03 2024, Brian Kubisiak wrote:
Toggle quote (10 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).

Ah, I see. That sounds good to me.

Given the channel is also being added to %load-path, I expect you can
add a command by defining (guix scripts foo) or (guix extensions foo)
and it will work either way (finding the former first).

We should document this method of extension in the manual. I think a new
section in the "(guix) Channels" chapter for "Declaring Custom Commands"
would be helpful. I might have some time to write something in the next
few days, but if you get a chance before then go ahead.

Thanks for your work on this,

Carlo
Brian Kubisiak wrote 3 months ago
[PATCH v2] ui: Search channels for guix extensions
(address . 74633@debbugs.gnu.org)
2c078699007e847611b7a20679621813691d9db3.1733695255.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 | 17 +++++++++++++++++
guix/self.scm | 1 +
guix/ui.scm | 12 +++++++++---
4 files changed, 31 insertions(+), 6 deletions(-)

Toggle diff (111 lines)
diff --git a/gnu/packages.scm b/gnu/packages.scm
index 1af3b8d440..42f3546606 100644
--- a/gnu/packages.scm
+++ b/gnu/packages.scm
@@ -148,15 +148,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..6694b1eaf7 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,21 @@ (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 ()
+ "Automatically add channels to Guile's search path. Channels are added
+to the end of the path so they don't override Guix' own modules. This
+function ensures that channels are only added to the search path once even if
+it is called multiple times."
+ (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..87ae24ea55 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,14 @@ (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?
+ (parse-path
+ (getenv "GUIX_EXTENSIONS_PATH")
+ (map
+ (cut string-append <> "/guix/extensions")
+ channels)))))

(define (commands)
"Return the list of commands, alphabetically sorted."

base-commit: 9001514e242ad15c190588439930b0fa4f6782e3
prerequisite-patch-id: cd0707c90e1d321f3f16f2f861313dd330e9f0b4
--
2.46.0
Ludovic Courtès wrote 3 months ago
(name . Brian Kubisiak)(address . brian@kubisiak.com)(address . 74633@debbugs.gnu.org)(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)
87plle17a3.fsf@gnu.org
Hi Brian,

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

Overall LGTM. I tested it with ‘make as-derivation’ and it works as
advertised; ‘strace -c’ shows that the number of syscalls is comparable
to that we currently have.

A couple of minor comments:

Toggle quote (8 lines)
> +++ b/gnu/packages.scm
> @@ -148,15 +148,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)))

This variable is now unused; I think it can be removed.

Toggle quote (15 lines)
> +(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 ()
> + "Automatically add channels to Guile's search path. Channels are added
> +to the end of the path so they don't override Guix' own modules. This
> +function ensures that channels are only added to the search path once even if
> +it is called multiple times."
> + (force promise))))

For clarity, I would call this ‘append-channels-to-load-path!’.

Using a promise here works, but I find it slightly misleading (because
we’re using it for side effects) and a bit heavyweight (promises are
thread-safe, so there’s a mutex etc.).

How about this:

(define (append-channels-to-load-path!)
(let-values (…)
…)
(set! append-channels-to-load-path! (lambda () #t)))

?

Could you send a v3 along these lines, if you think that makes sense?

Thanks,
Ludo’.
Ludovic Courtès wrote 3 months ago
control message for bug #74633
(address . control@debbugs.gnu.org)
87msgiyvs2.fsf@gnu.org
merge 74633 74425
quit
Brian Kubisiak wrote 2 months ago
[PATCH v3] ui: Search channels for guix extensions
(address . 74633@debbugs.gnu.org)
6a1ff2a779e9cf5248671fc9e62852aa23be3f16.1736299302.git.brian@kubisiak.com
Attachment: file
Brian Kubisiak wrote 2 months ago
Re: [bug#74633] [PATCH v2] ui: Search channels for guix extensions
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 74633@debbugs.gnu.org)(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)
4ke6rxdjqgco4quclch6erzjtkz33pz6ofzbh6xzgshpc3us63@ohfmoaq2dnzg
Toggle quote (2 lines)
> Could you send a v3 along these lines, if you think that makes sense?

Sent. Note that during testing I noticed that guile-git (and
guile-json-4) needed to be added as dependencies to a g-expression
that imported guix/build/utils.scm due to the additional dependencies
introduced in this patch. I'm probably missing more places where this
is required and Carlo's lazy import may be a better option to avoid
issues like this. Let me know what you think, I can send another
version if you think that's better.

Toggle quote (10 lines)
> > +++ b/gnu/packages.scm
> > @@ -148,15 +148,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)))
>
> This variable is now unused; I think it can be removed.

This is still used in the below make-parameter call (not shown in the
patch), so I've left it in.

Thanks,
Brian
Ludovic Courtès wrote 2 months ago
Re: [bug#74633] [PATCH v3] ui: Search channels for guix extensions
(name . Brian Kubisiak)(address . brian@kubisiak.com)(address . 74633@debbugs.gnu.org)
87v7umjxug.fsf@gnu.org
Hello,

Overall it LGTM. I propose the mostly-cosmetic changes below.

Once thing I overlooked before is that commands will have to live under
/guix/extensions, right?

(define (commands)
"Return the list of commands, alphabetically sorted."
(filter-map source-file-command
(append (command-files)
(append-map command-files
(extension-directories)))))

And likewise in ‘run-guix-command’.

But now, if a channel provides ‘guix/scripts/foo.scm’, the ‘guix help’
command will not show ‘foo’ but the ‘guix foo’ command will effectively
work (which wasn’t the case until now).

Maybe it’s fine actually, I don’t know, but I thought this is worth
mentioning and thinking though.

WDYT?

Ludo’.
Toggle diff (56 lines)
diff --git a/guix/describe.scm b/guix/describe.scm
index 90c17084d1..819f0fef74 100644
--- a/guix/describe.scm
+++ b/guix/describe.scm
@@ -27,8 +27,8 @@ (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 (srfi srfi-71)
#:use-module (ice-9 match)
#:export (current-profile
current-profile-date
@@ -194,10 +194,11 @@ (define (package-path-entries)
(define (append-channels-to-load-path!)
"Automatically add channels to Guile's search path. Channels are added to the
-end of the path so they don't override Guix' own modules. This function ensures
-that channels are only added to the search path once even if it is called
-multiple times."
- (let-values (((channels-scm channels-go) (package-path-entries)))
+end of the path so they don't override Guix' own modules.
+
+This procedure ensures that channels are only added to the search path once
+even if it is called multiple times."
+ (let ((channels-scm channels-go (package-path-entries)))
(set! %load-path
(append %load-path channels-scm))
(set! %load-compiled-path
diff --git a/guix/ui.scm b/guix/ui.scm
index 05bc99a7e3..d9d7c8469f 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -38,7 +38,8 @@
(define-module (guix ui) ;import in user interfaces only
#:use-module (guix i18n)
#:use-module (guix colors)
- #:use-module (guix describe)
+ #:autoload (guix describe) (append-channels-to-load-path!
+ package-path-entries)
#:use-module (guix diagnostics)
#:use-module (guix gexp)
#:use-module (guix sets)
@@ -2200,9 +2201,8 @@ (define (extension-directories)
(filter file-exists?
(parse-path
(getenv "GUIX_EXTENSIONS_PATH")
- (map
- (cut string-append <> "/guix/extensions")
- channels)))))
+ (map (cut string-append <> "/guix/extensions")
+ channels)))))
(define (commands)
"Return the list of commands, alphabetically sorted."
Brian Kubisiak wrote 2 months ago
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 74633@debbugs.gnu.org)
cbp4zxxxk3ygqabe7xevpoiinimcgovg3f7zjvxemwzskyx4ap@4w5hm2xaeg74
Toggle quote (2 lines)
> Overall it LGTM. I propose the mostly-cosmetic changes below.

LGTM

Toggle quote (19 lines)
> Once thing I overlooked before is that commands will have to live under
> /guix/extensions, right?
>
> (define (commands)
> "Return the list of commands, alphabetically sorted."
> (filter-map source-file-command
> (append (command-files)
> (append-map command-files
> (extension-directories)))))
>
> And likewise in ‘run-guix-command’.
>
> But now, if a channel provides ‘guix/scripts/foo.scm’, the ‘guix help’
> command will not show ‘foo’ but the ‘guix foo’ command will effectively
> work (which wasn’t the case until now).
>
> Maybe it’s fine actually, I don’t know, but I thought this is worth
> mentioning and thinking though.

My intention was to use /guix/extensions since it's the same directory
structure as existing extensions. I don't have a strong preference
here, though I think Carlo intended to support /guix/scripts in
#74425. Maybe it's a good idea to support both?

Brian
Ludovic Courtès wrote 2 months ago
(name . Brian Kubisiak)(address . brian@kubisiak.com)(address . 74633@debbugs.gnu.org)(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)
87jzajdese.fsf@gnu.org
Hi,

Brian Kubisiak <brian@kubisiak.com> skribis:

Toggle quote (24 lines)
>> Once thing I overlooked before is that commands will have to live under
>> /guix/extensions, right?
>>
>> (define (commands)
>> "Return the list of commands, alphabetically sorted."
>> (filter-map source-file-command
>> (append (command-files)
>> (append-map command-files
>> (extension-directories)))))
>>
>> And likewise in ‘run-guix-command’.
>>
>> But now, if a channel provides ‘guix/scripts/foo.scm’, the ‘guix help’
>> command will not show ‘foo’ but the ‘guix foo’ command will effectively
>> work (which wasn’t the case until now).
>>
>> Maybe it’s fine actually, I don’t know, but I thought this is worth
>> mentioning and thinking though.
>
> My intention was to use /guix/extensions since it's the same directory
> structure as existing extensions. I don't have a strong preference
> here, though I think Carlo intended to support /guix/scripts in
> #74425. Maybe it's a good idea to support both?

Yes, it’s probably a good idea to support both. In that case, the only
thing missing here I think is for ‘commands’ to somehow visit
guix/scripts/*.scm coming from channels.

Thanks,
Ludo’.
Brian Kubisiak wrote 4 weeks ago
[PATCH v4] ui: Search channels for guix extensions
(address . 74633@debbugs.gnu.org)
6c9bd54fc741c8e31821b1506e261776b0829e43.1739806839.git.brian@kubisiak.com
Attachment: file
Brian Kubisiak wrote 4 weeks ago
Re: [bug#74633] [PATCH v3] ui: Search channels for guix extensions
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 74633@debbugs.gnu.org)(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)
5kgsxrxuoov5bouozoozxqqow6kanf3bsqhfmywnofcpoesbzg@obx3z4sbkfn2
Toggle quote (9 lines)
> > My intention was to use /guix/extensions since it's the same directory
> > structure as existing extensions. I don't have a strong preference
> > here, though I think Carlo intended to support /guix/scripts in
> > #74425. Maybe it's a good idea to support both?
>
> Yes, it’s probably a good idea to support both. In that case, the only
> thing missing here I think is for ‘commands’ to somehow visit
> guix/scripts/*.scm coming from channels.

I've sent an updated v4 (sorry for the long delay!) that addresses
this as well as the style fixes you suggested above.

I've also continued to run into issues with trying to import (or
autoload) the 'guix describe' module from guix/ui.scm and have
resorted to Carlo's original method of resolving the symbols lazily to
avoid the issue.

Thanks,
Brian
Ludovic Courtès wrote 7 days ago
Re: [bug#74633] [PATCH v4] ui: Search channels for guix extensions
(name . Brian Kubisiak)(address . brian@kubisiak.com)(address . 74633-done@debbugs.gnu.org)
878qpftr2w.fsf@gnu.org
Hi Brian,

Brian Kubisiak <brian@kubisiak.com> skribis:

Toggle quote (10 lines)
> * guix/describe.scm (append-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

Finally applied, with the cosmetic changes below.

Thanks!

Ludo’.
Toggle diff (21 lines)
diff --git a/guix/ui.scm b/guix/ui.scm
index 3cc15b05fc..d462f7133e 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -2197,11 +2197,12 @@ (define (extension-directories)
;; We need to resolve these lazily, because even using an #:autoload is too
;; much and breaks compilation during "guix pull".
(define append-channels-to-load-path!
- (module-ref (resolve-module `(guix describe))
- (symbol-append 'append-channels-to-load-path!)))
+ (module-ref (resolve-interface '(guix describe))
+ 'append-channels-to-load-path!))
(define package-path-entries
- (module-ref (resolve-module `(guix describe))
- (symbol-append 'package-path-entries)))
+ (module-ref (resolve-interface '(guix describe))
+ 'package-path-entries))
+
(append-channels-to-load-path!)
(let ((channels (package-path-entries)))
(filter file-exists?
Closed
?
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
You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags
mumi command -t +confirmed -t +easy
Or, remove the moreinfo tag and set the help tag
mumi command -t -moreinfo -t +help