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: