[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Merging guix.el
Re: Merging guix.el
Fri, 29 Aug 2014 22:24:55 +0400
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)
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?
>>> 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
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’.
>>> 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
>> 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
> 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)))))
> 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))
> (if (vhash-assq name result)
... 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:
(%manifest entries name->entries)
(entries manifest-entries) ; list of <manifest-entry>
(name->entries manifest-name->entries)) ; vhash [string -> list of
(define (manifest entries)
(fold (lambda (entry result)
(vhash-cons ... result))
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.
Re: Merging guix.el, Alex Kost, 2014/08/27