Re: [PATCH -v2 2/2] guix: profiles: create fonts.dir/scale for all fonts directories
l...@gnu.org (Ludovic Courtès) writes: > huang ying skribis: > >> On Wed, Mar 8, 2017 at 4:24 AM, Danny Milosavljevic >> wrote: >>> Hi, >>> +(with-directory-excursion dir + (and (file-exists? fonts-scale-file) + (delete-file fonts-scale-file)) + (and (file-exists? fonts-dir-file) + (delete-file fonts-dir-file)) + (system* mkfontscale) + (system* mkfontdir) >>> >>> Please do not throw away the status code here (result of system*). You can >>> check for okayness by (zero? (system* ...)). >> >> Then what is the intended behavior? abort the build process with >> message and non-zero exit code? Usually we will raise a exception or >> just display some message and exit? > > See for instance ‘info-dir-file’, which does this: > > (exit (every install-info >(append-map info-files >'#$(manifest-inputs manifest > > The effect is to exit with 0 upon success and some other code upon > failure, leading to a proper derivation build failure. Sure. Best Regards, Huang, Ying > HTH! > > Ludo’.
Re: [PATCH -v2 2/2] guix: profiles: create fonts.dir/scale for all fonts directories
Hi, Ludo, Thanks for comments! l...@gnu.org (Ludovic Courtès) writes: > Huang Ying skribis: > >> * guix/profiles.scm (fonts-dir-file): Create fonts.dir/scale files for all >> fonts directories. > > Looks cool, modulo Danny’s suggestions and minor issues below. > > BTW, could you explain (probably as a comment in the code) why we need > to do all this? What is it fixing? :-) Sure. >> +(union-build fonts-dir fonts-dirs >> + #:log-port (%make-void-port "w") >> + #:create-all-directories? #t) >> +(ftw fonts-dir >> + (lambda (dir statinfo flag) > > ‘ftw’ is not great IMO and not used in Guix. > > I would suggest using ‘find-files’ from (guix build utils). You could > write something like: > > (let ((directories (find-files fonts-dir > (lambda (file stat) >(eq? 'directory (stat:type stat))) > #:directories? #t))) > (for-each do-something directories)) > > This also has the advantage of separating the generation of the > directory list from the actual action. Sure. >> + (and (eq? flag 'directory) >> +(with-directory-excursion dir >> + (and (file-exists? fonts-scale-file) >> + (delete-file fonts-scale-file)) > > Here, since ‘delete-file’ has a side-effect and a meaningless return > value, we conventionally avoid ‘and’ and instead write: > > (when (file-exists? fonts-scale-file) > (delete-file fonts-scale-file)) > > for clarity. Sure >> + (and (file-exists? fonts-dir-file) >> + (delete-file fonts-dir-file)) >> + (system* mkfontscale) >> + (system* mkfontdir) >> + (and (empty-file? fonts-scale-file) >> + (delete-file fonts-scale-file)) >> + (and (empty-file? fonts-dir-file) >> + (delete-file fonts-dir-file > > Same as above. Sure. Best Regards, Huang, Ying > Thank you! > > Ludo’.
Re: [PATCH -v2 2/2] guix: profiles: create fonts.dir/scale for all fonts directories
Huang Ying skribis: > * guix/profiles.scm (fonts-dir-file): Create fonts.dir/scale files for all > fonts directories. Looks cool, modulo Danny’s suggestions and minor issues below. BTW, could you explain (probably as a comment in the code) why we need to do all this? What is it fixing? :-) > +(union-build fonts-dir fonts-dirs > + #:log-port (%make-void-port "w") > + #:create-all-directories? #t) > +(ftw fonts-dir > + (lambda (dir statinfo flag) ‘ftw’ is not great IMO and not used in Guix. I would suggest using ‘find-files’ from (guix build utils). You could write something like: (let ((directories (find-files fonts-dir (lambda (file stat) (eq? 'directory (stat:type stat))) #:directories? #t))) (for-each do-something directories)) This also has the advantage of separating the generation of the directory list from the actual action. > + (and (eq? flag 'directory) > +(with-directory-excursion dir > + (and (file-exists? fonts-scale-file) > + (delete-file fonts-scale-file)) Here, since ‘delete-file’ has a side-effect and a meaningless return value, we conventionally avoid ‘and’ and instead write: (when (file-exists? fonts-scale-file) (delete-file fonts-scale-file)) for clarity. > + (and (file-exists? fonts-dir-file) > + (delete-file fonts-dir-file)) > + (system* mkfontscale) > + (system* mkfontdir) > + (and (empty-file? fonts-scale-file) > + (delete-file fonts-scale-file)) > + (and (empty-file? fonts-dir-file) > + (delete-file fonts-dir-file Same as above. Thank you! Ludo’.
Re: [PATCH -v2 2/2] guix: profiles: create fonts.dir/scale for all fonts directories
huang ying skribis: > On Wed, Mar 8, 2017 at 4:24 AM, Danny Milosavljevic > wrote: >> Hi, >> >>> +(with-directory-excursion dir >>> + (and (file-exists? fonts-scale-file) >>> + (delete-file fonts-scale-file)) >>> + (and (file-exists? fonts-dir-file) >>> + (delete-file fonts-dir-file)) >>> + (system* mkfontscale) >>> + (system* mkfontdir) >> >> Please do not throw away the status code here (result of system*). You can >> check for okayness by (zero? (system* ...)). > > Then what is the intended behavior? abort the build process with > message and non-zero exit code? Usually we will raise a exception or > just display some message and exit? See for instance ‘info-dir-file’, which does this: (exit (every install-info (append-map info-files '#$(manifest-inputs manifest The effect is to exit with 0 upon success and some other code upon failure, leading to a proper derivation build failure. HTH! Ludo’.
Re: [PATCH -v2 2/2] guix: profiles: create fonts.dir/scale for all fonts directories
On Wed, Mar 8, 2017 at 4:24 AM, Danny Milosavljevic wrote: > Hi, > >> +(with-directory-excursion dir >> + (and (file-exists? fonts-scale-file) >> + (delete-file fonts-scale-file)) >> + (and (file-exists? fonts-dir-file) >> + (delete-file fonts-dir-file)) >> + (system* mkfontscale) >> + (system* mkfontdir) > > Please do not throw away the status code here (result of system*). You can > check for okayness by (zero? (system* ...)). Then what is the intended behavior? abort the build process with message and non-zero exit code? Usually we will raise a exception or just display some message and exit? Best Regards, Huang, Ying
Re: [PATCH -v2 2/2] guix: profiles: create fonts.dir/scale for all fonts directories
Hi, > +(with-directory-excursion dir > + (and (file-exists? fonts-scale-file) > + (delete-file fonts-scale-file)) > + (and (file-exists? fonts-dir-file) > + (delete-file fonts-dir-file)) > + (system* mkfontscale) > + (system* mkfontdir) Please do not throw away the status code here (result of system*). You can check for okayness by (zero? (system* ...)).