Re: Merging guix.el

[ludo@gnu.org (Ludovic Courtès)]

[-- Attachment #1: Type: text/plain, Size: 3450 bytes --]

Alex Kost <alezost@gmail.com> skribis:

> Ludovic Courtès (2014-08-23 16:17 +0400) wrote:

[...]

>>   • Have them appropriately listed in the top-level Makefile.am (I can
>>     help with that, if you’re not familiar.)
>
> Along with the small changes to top-level "Makefile.am", I made
> "Makefile.am" in "emacs" dir and...

I think it’s better to avoid recursive makefiles, which is why I
suggested adding changes to the top-level Makefile.am.

Could you make it this way?

More precisely, the Emacs-specific things could be kept in emacs.am, and
that file would be included from the top-level Makefile.am.

> I imagine there may be... for example vi users, who wouldn't want to
> install this feature, so I made some changes in "configure.ac" to add
> “--disable-emacs-ui” option.

It seems that the only things that cannot be done when Emacs is not
available is the generation of the autoloads file, right?

Then, what about adding $(AUTOLOADS) to the distribution?  It would just
need to be appended to dist_lisp_DATA, and not added to CLEANFILES.

Nitpick: could you use makefile-backslash-region for the $(AUTOLOADS)
recipe?

> Also I use almost the same code in "guix-helper.scm.in" that is used in
> "scripts/guix.in", so I think it will be good to have some little
> additional module with ‘config-lookup’ function.  WDYT?

It cannot be in a module, because at this point the module location
isn’t known yet.  I don’t really know how to factorize it, so I propose
to leave it for later, with a FIXME.  Maybe Mark has an idea?

+(define %current-manifest)
+(define current-manifest-entries-table)
+(define packages)
+(define packages-table)

I didn’t know this was possible, but we shouldn’t rely on it.

+(define name+version->key cons)
+(define (key->name+version key)
+  (values (car key) (cdr key)))

I would find it easier to read if it ‘cons’ and ‘car+cdr’ (from SRFI-1)
were used directly.

+(define* (set-current-manifest-maybe! #:optional manifest)
+  (define (manifest-entries->hash-table entries)
+    (let ((entries-table (make-hash-table (length entries))))
+      (map (lambda (entry)
+             (let* ((key (name+version->key
+                          (manifest-entry-name entry)
+                          (manifest-entry-version entry)))
+                    (ref (hash-ref entries-table key)))
+               (hash-set! entries-table key
+                          (if ref (cons entry ref) (list entry)))))
+           entries)
+      entries-table))
+
+  (let ((manifest (or manifest (profile-manifest %user-profile))))
+    (unless (and (manifest? %current-manifest)
+                 (equal? manifest %current-manifest))
+      (set! %current-manifest manifest)
+      (set! current-manifest-entries-table
+            (manifest-entries->hash-table
+             (manifest-entries manifest))))))

Wouldn’t it be enough to pass the current manifest as an argument to the
various functions, instead of defining a global variable?

Also, my understanding is that ‘current-manifest-entries-table’ is here
to speed up lookups in ‘manifest-entries-by-name+version’, right?

Then, I think this optimization should go into (guix profiles):
<manifest> objects would carry that vhash, and ‘manifest-installed?’
etc. would make use of it.  The constructor would be changed along these
lines:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 767 bytes --]

diff --git a/guix/profiles.scm b/guix/profiles.scm
index a2c73fd..98eb814 100644
--- a/guix/profiles.scm
+++ b/guix/profiles.scm
@@ -84,9 +84,17 @@
 ;;;
 
 (define-record-type <manifest>
-  (manifest entries)
+  (%manifest entries name->entry)
   manifest?
-  (entries manifest-entries))                     ; list of <manifest-entry>
+  (entries     manifest-entries)         ; list of <manifest-entry>
+  (name->entry manifest-name->entry))    ; vhash [string -> <manifest-entry>]
+
+(define (manifest entries)
+  (%manifest entries
+             (fold (lambda (entry result)
+                     (vhash-cons ... result))
+                   vlist-null
+                   entries)))
 
 ;; Convenient alias, to avoid name clashes.
 (define make-manifest manifest)

[-- Attachment #3: Type: text/plain, Size: 1974 bytes --]


WDYT?

+(define (set-packages!)
+  (let ((count 0))
+    (set! packages
+          (fold-packages (lambda (pkg res)
+                           (set! count (+ 1 count))
+                           (vhash-consq (object-address pkg) pkg res))
+                         vlist-null))
+    (set! packages-table (make-hash-table count))
+    (vlist-for-each (lambda (elem)
+                      (let* ((pkg (cdr elem))
+                             (key (name+version->key
+                                   (package-name pkg)
+                                   (package-version pkg)))
+                             (ref (hash-ref packages-table key)))
+                        (hash-set! packages-table key
+                                   (if ref (cons pkg ref) (list pkg)))))
+                    packages)))
+
+(set-packages!)

Given that ‘set-packages!’ has only on call site, what about removing
it, and instead writing directly:

  (define %packages
    (fold-packages ... vlist-null))

  (define %package-count
    (length %packages))

  (define %package-table
    (vlist-fold ...))

It’s also best to prefix global variable names with ‘%’.

I should point out that we try to stick to functional style (in
host-side code at least.)  Thus any identifier with an exclamation mark
gives you a -1 during review.  ;-)  See “Coding Style” in HACKING.

>>   • Add a section in the manual, probably under “Package Management”,
>>     describing the interface (probably as a separate .texi file
>>     @included from the main one.)  It can come later.
>
> This is new subject for me as well; I'll write it as soon as possible.
>
> For now the most important part of the documentation is that after
> installing guix with emacs UI, it may be used by putting:
>
>   (require 'guix-init)
>
> into ".emacs".  After that autoloaded "guix-..." commands should be
> available.

Cool!

Thanks,
Ludo’.