Re: [PATCH -v2 2/2] guix: profiles: create fonts.dir/scale for all fonts directories

[Huang, Ying]

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

[Huang, Ying]

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

[Ludovic Courtès]

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

[Ludovic Courtès]

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

[huang ying]

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

[Danny Milosavljevic]

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* ...)).