Re: Merging guix.el

[Alex Kost]

Hello,

Ludovic Courtès (2014-08-29 00:09 +0400) wrote:

> Alex Kost <address@hidden> skribis:
>
>> Ludovic Courtès (2014-08-28 16:41 +0400) wrote:
>>
>>> Alex Kost <address@hidden> skribis:

[...]

>>>> 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?
>>
>> Yes.
>>
>>> Then, what about adding $(AUTOLOADS) to the distribution?  It would just
>>> need to be appended to dist_lisp_DATA, and not added to CLEANFILES.
>>
>> OK, will be done.  And what about "configure.ac"?  I thought a new
>> configure option is good.  Should I delete it?
>
> I think so, yes, because it wouldn’t make any difference if the
> autoloads are already generated.  Non-Emacs users would end up
> installing a few files that they would not use, but that’s not big deal
> IMO.

OK, I made "emacs.am" and modified "configure.ac" and "Makefile.am" (I
pushed 2 new commits to “emacs-ui” branch).  Is there anything else to
be done in a build part?

>
> [...]
>
>>> Also, my understanding is that ‘current-manifest-entries-table’ is here
>>> to speed up lookups in ‘manifest-entries-by-name+version’, right?
>>
>> Yes, ‘current-manifest-entries-table’ and ‘packages-table’ are there to
>> speed up the process of finding “manifest entries”/“packages” by
>> name+version (it is a very general need).  Also
>> ‘current-manifest-entries-table’ is used in ‘fold-manifest-entries’.
>
> OK.
>
>>> 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:
>
> [...]
>
>> I think it's a good idea, but if that "name" is just a package name,
>> I can't use this optimization: I need to define entries by
>> "name+version".
>
> Yes, makes sense.  So that ‘name->entries’ field would map a package
> name to a list of entries; in the most common case, there’ll be just one
> entry anyway.  How does that sound?

Yes, I think I could use it; I have a problem however.  I don't know how
to use vhash for that (see a comment for %package-table below); if
hash-table is OK then I can make it.

Also I'm still not sure about the name ‘manifest-name->entries’ (or
‘manifest-name->entry’), as this function returns a vhash, it does not
transform a name into a list of entries.  It may be confusing, no?

>
> [...]
>
>>> 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 recalled why I used that ugly ‘set-packages!’: I just didn't want to
count packages again (i.e. to use ‘length’) for hash-table, so I
combined setting ‘packages’ variable with counting.  For now I got rid
of ‘set-packages!’, as you suggested with one exception...

>> Yes, it's definitely better, except I don't know how to fill a table
>> with ‘vlist-fold’ and I don't see a better variant than this:
>>
>> (define %package-table
>>   (let ((table (make-hash-table %package-count)))
>>     (vlist-for-each (lambda (elem)
>>                       (let* ((pkg (cdr elem))
>>                              (key (name+version->key
>>                                    (package-name pkg)
>>                                    (package-version pkg)))
>>                              (ref (hash-ref table key)))
>>                         (hash-set! table key
>>                                    (if ref (cons pkg ref) (list pkg)))))
>>                     packages)
>>     table))
>
> I would make %package-table a vhash instead of a hash table:
>
>   (define %package-table
>     (vlist-fold (lambda (elem result)
>                   (match elem
>                     ((name . package)
>                      (vhash-cons (cons (package-name package)
>                                        (package-version package))
>                                  package
>                                  (if (vhash-assq name result)
>                                      ...)))))
>                 vlist-null
>                 %packages))

... I left hash table, because I still don't understand how to use vhash
for that: a name+version key should give a list of all matching
packages, but AFAIU your variant would just replace one package with
another (with the same name+version).

The same thing with modifications in <manifest> that you proposed:

(define-record-type <manifest>
  (%manifest entries name->entries)
  manifest?
  (entries       manifest-entries)        ; list of <manifest-entry>
  (name->entries manifest-name->entries)) ; vhash [string -> list of 
<manifest-entry>]

(define (manifest entries)
  (%manifest entries
             (fold (lambda (entry result)
                     (vhash-cons ... result))
                   vlist-null
                   entries)))

I can't see how to build an appropriate vhash, sorry.  Need help :)

As for ‘set-current-manifest-maybe!’, I'm afraid it can't be deleted.  I
found that I put it in some places where it shouldn't appear and I fixed
that, but it still stays in functions returning “package information”
for the elisp side (for info/list buffers).  Currently I can't invent a
way how to get rid of this function completely.

--
Alex