diff mbox series

[bug#50755,v2] import: Generate list of importers based on available modules

Message ID 87wnn7ouq3.fsf@airmail.cc
State New
Headers show
Series [bug#50755,v2] import: Generate list of importers based on available modules | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/issue success View issue
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/issue success View issue

Commit Message

pinoaffe Sept. 23, 2021, 9:19 p.m. UTC 
* guix/scripts/import.scm (importers): Generate a list of all importers by
looping over available guile modules, allowing for extensibility.
---
 guix/scripts/import.scm | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Simon Tournier Sept. 27, 2021, 2:28 p.m. UTC | #1 
Hi,

Thanks.  Two comments.

On Thu, 23 Sept 2021 at 23:20, pinoaffe <pinoaffe@airmail.cc> wrote:

> -(define importers '("gnu" "pypi" "cpan" "hackage" "stackage" "egg" "elpa"
> -                    "gem" "go" "cran" "crate" "texlive" "json" "opam"
> -                    "minetest"))
> +(define importers (filter-map (lambda (module)
> +                                (match (module-name module)
> +                                  (`(guix scripts import ,importer)
> +                                   (symbol->string importer))
> +                                  ( #t #f)))
> +                              (all-modules (map (lambda (entry)
> +                                                  `(,entry . "guix/scripts/import"))
> +                                                %load-path))))
>
>  (define (resolve-importer name)
>    (let ((module (resolve-interface

First, I think, it breaks "guix import --help".  Therefore, this patch
needs a v3. :-)

Second, what is the average extra time added on cold cache?  On my
machine, for hot cache, I get:

--8<---------------cut here---------------start------------->8---
$ time guix import cran -h

real    0m0.113s
user    0m0.110s
sys    0m0.025s

$ time ./pre-inst-env guix import cran -h

real    0m0.470s
user    0m0.529s
sys    0m0.054s
--8<---------------cut here---------------end--------------->8---

which is something.  On cold cache, it is:

real    0m10.438s
user    0m0.164s
sys    0m0.082s

vs

real    0m12.226s
user    0m0.897s
sys    0m0.190s

but these numbers are not so much meaningful because there is a strong
variability; hence on average. :-)

Because of 'filter-map', it walks all the modules, so there is a
performance loss.  The question is: which performance loss is
acceptable here?
Other said, is the code improvement worth compared to the performance decrease?

All the best,
simon
pinoaffe Sept. 27, 2021, 6:27 p.m. UTC | #2 
Hi,

thank you for your feedback!

zimoun writes:
> First, I think, it breaks "guix import --help".  Therefore, this patch
> needs a v3. :-)
I sent a v3, thanks :)

> The question is: which performance loss is acceptable here?
To me a performance loss similar to the one you describe would be
acceptable, particularly since it should be a constant performance hit
for every time ~guix import~ is called, aka it won't significantly
impact long-running import commands. However, I think I may be somewhat
biased on this one, so it'd be great if others could weigh in :)

Kind regards,
pinoaffe
diff mbox series

Patch

diff --git a/guix/scripts/import.scm b/guix/scripts/import.scm
index 40fa6759ae..ed702d3bff 100644
--- a/guix/scripts/import.scm
+++ b/guix/scripts/import.scm
@@ -23,6 +23,7 @@ 
 
 (define-module (guix scripts import)
   #:use-module (guix ui)
+  #:use-module (guix discovery)
   #:use-module (guix scripts)
   #:use-module (guix utils)
   #:use-module (srfi srfi-1)
@@ -78,9 +79,14 @@  rather than \\n."
 ;;; Entry point.
 ;;;
 
-(define importers '("gnu" "pypi" "cpan" "hackage" "stackage" "egg" "elpa"
-                    "gem" "go" "cran" "crate" "texlive" "json" "opam"
-                    "minetest"))
+(define importers (filter-map (lambda (module)
+                                (match (module-name module)
+                                  (`(guix scripts import ,importer)
+                                   (symbol->string importer))
+                                  ( #t #f)))
+                              (all-modules (map (lambda (entry)
+                                                  `(,entry . "guix/scripts/import"))
+                                                %load-path))))
 
 (define (resolve-importer name)
   (let ((module (resolve-interface