From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Kost Subject: Re: Merging guix.el Date: Fri, 29 Aug 2014 22:24:55 +0400 Message-ID: <87iolbkvbc.fsf@gmail.com> References: <874mx3z9h2.fsf@gnu.org> <87fvgim60p.fsf@gmail.com> <87mwaobxbo.fsf@gnu.org> <87tx4wlbis.fsf@gmail.com> <87wq9s4bqy.fsf@gnu.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:52081) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XNQr9-0008Vh-LX for guix-devel@gnu.org; Fri, 29 Aug 2014 14:25:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XNQr4-0007GE-T0 for guix-devel@gnu.org; Fri, 29 Aug 2014 14:25:03 -0400 In-Reply-To: <87wq9s4bqy.fsf@gnu.org> ("Ludovic \=\?utf-8\?Q\?Court\=C3\=A8s\=22'\?\= \=\?utf-8\?Q\?s\?\= message of "Thu, 28 Aug 2014 22:09:41 +0200") List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: Ludovic =?utf-8?Q?Court=C3=A8s?= Cc: Guix-devel --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hello, Ludovic Court=C3=A8s (2014-08-29 00:09 +0400) wrote: > Alex Kost skribis: > >> Ludovic Court=C3=A8s (2014-08-28 16:41 +0400) wrote: >> >>> Alex Kost 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 >>>> =E2=80=9C--disable-emacs-ui=E2=80=9D 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=E2=80=99t 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=E2=80=99s not bi= g deal > IMO. OK, I made "emacs.am" and modified "configure.ac" and "Makefile.am" (I pushed 2 new commits to =E2=80=9Cemacs-ui=E2=80=9D branch). Is there anyth= ing else to be done in a build part? > > [...] > >>> Also, my understanding is that =E2=80=98current-manifest-entries-table= =E2=80=99 is here >>> to speed up lookups in =E2=80=98manifest-entries-by-name+version=E2=80= =99, right? >> >> Yes, =E2=80=98current-manifest-entries-table=E2=80=99 and =E2=80=98packa= ges-table=E2=80=99 are there to >> speed up the process of finding =E2=80=9Cmanifest entries=E2=80=9D/=E2= =80=9Cpackages=E2=80=9D by >> name+version (it is a very general need). Also >> =E2=80=98current-manifest-entries-table=E2=80=99 is used in =E2=80=98fol= d-manifest-entries=E2=80=99. > > OK. > >>> Then, I think this optimization should go into (guix profiles): >>> objects would carry that vhash, and =E2=80=98manifest-instal= led?=E2=80=99 >>> 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 =E2=80=98name->entries=E2=80=99 field would ma= p a package > name to a list of entries; in the most common case, there=E2=80=99ll be j= ust 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 =E2=80=98manifest-name->entries=E2= =80=99 (or =E2=80=98manifest-name->entry=E2=80=99), as this function returns a vhash, = it does not transform a name into a list of entries. It may be confusing, no? > > [...] > >>> Given that =E2=80=98set-packages!=E2=80=99 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=E2=80=99s also best to prefix global variable names with =E2=80=98%= =E2=80=99. I recalled why I used that ugly =E2=80=98set-packages!=E2=80=99: I just did= n't want to count packages again (i.e. to use =E2=80=98length=E2=80=99) for hash-table,= so I combined setting =E2=80=98packages=E2=80=99 variable with counting. For no= w I got rid of =E2=80=98set-packages!=E2=80=99, as you suggested with one exception... >> Yes, it's definitely better, except I don't know how to fill a table >> with =E2=80=98vlist-fold=E2=80=99 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 that you proposed: --=-=-= Content-Type: text/plain Content-Disposition: inline; filename=sample.scm (define-record-type (%manifest entries name->entries) manifest? (entries manifest-entries) ; list of (name->entries manifest-name->entries)) ; vhash [string -> list of ] (define (manifest entries) (%manifest entries (fold (lambda (entry result) (vhash-cons ... result)) vlist-null entries))) --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable I can't see how to build an appropriate vhash, sorry. Need help :) As for =E2=80=98set-current-manifest-maybe!=E2=80=99, I'm afraid it can't b= e 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 =E2=80=9Cpackage informatio= n=E2=80=9D for the elisp side (for info/list buffers). Currently I can't invent a way how to get rid of this function completely. -- Alex --=-=-=--