Re: [PATCH 1/2] build: union: Add create-all-directory? parameter to union-build

[Mark H Weaver]

Danny Milosavljevic  writes:

> On Fri, 24 Feb 2017 17:13:53 -0500
> Mark H Weaver  wrote:
>
>> If the purpose here is to facilitate mutating anything in the store,
>
> That's not what it does. It's used in the profile hook. 'union-build
> is basically just reused to avoid code duplication. See PATCH 2/2.

I see now.  I withdraw my objection.  Sorry for the delay.

The only remaining issue I see is that the second patch removes error
checking on the status code returned by 'mkfontscale' and 'mkfontdir'.
It would be preferable to retain that.  However, it's not a serious
issue.

 Regards,
   Mark

Re: [PATCH 1/2] build: union: Add create-all-directory? parameter to union-build

[Huang, Ying]

iyzs...@member.fsf.org (宋文武) writes:

> "Huang, Ying"  writes:
>
> Hi, sorry for my late reply.
>
>> Danny Milosavljevic  writes:
>>
>>> Hi,
>>>
>>> yeah, I've looked at it and I see what you are trying to do.
>>>
>>> You modified 'union to replicate the directory tree and symlink
>>> only the non-directory files - in order to be able to add
>>> "fonts.dir" in those directories.
>>>
>>> The previous version symlinked entire directories, too, and you
>>> wouldn't have been able to add those "fonts.dir" files inside them
>>> later.
>>>
>>> Your fix seems kinda hacky - but I don't really see a downside.
>>>
>>> Thinking about it some more I think it looks good enough to me. I'd
>>> like a second opinion, though.
>>>
>>> Other than that, the name of the new flag "create-all-directory?"
>>> is weird. I'd like it to be something like
>>> "create-all-directories?".
>
>>>
>>> (The non-hacky solution would be to make 'union aware that there are
>>> "fonts.dir" files to go in there, too. One could do that by adding
>>> these as an input, too. I'm not sure whether that would be worth the
>>> added complexity, though - especially since the fonts.dir generator
>>> needs to have the finished union as input and not just single
>>> directories one by one. Thoughts?)
>>
>> union will only be called with create-all-directory? != nil for packages
>> have share/fonts directories.  And for these packages, we plan to remove
>> the fonts.dir and fonts.scale files, so that we need to create all
>> directories for them after all.  Is this good enough?
>>
>
> Yes, I thought so and it will work, but maybe it's better (non-hacky
> way?) to make every font package install fonts.scale and fonts.dir files
> into its own directory...
>
> IIUC, to make them useful, we have to make the fonts directories
> available to the running X server through xserver.conf (FontPath) or
> adding them using 'xset +fp'.  And since either method is limited to a
> single directory, so if we avoid the collision by putting each font
> package under an unique path ($out/share/fonts/TYPE/FONT-NAME), then the
> font-dir profile hook is not needed at all.

In the other hand, we can add these paths into xorg.conf or `xset +fp`
via font-dir profile hook too.

> We can do this in the wanted (upcoming?) 'font-build-system'.

One question, how to deal with font packages which aren't built with
font-build-system?  For example, font-terminus uses gnu-build-system.

Best Regards,
Huang, Ying

Re: [PATCH 1/2] build: union: Add create-all-directory? parameter to union-build

[Danny Milosavljevic]

Hi Mark,

On Fri, 24 Feb 2017 17:13:53 -0500
Mark H Weaver  wrote:

> If the purpose here is to facilitate mutating anything in the store,

That's not what it does. It's used in the profile hook. 'union-build is 
basically just reused to avoid code duplication. See PATCH 2/2.

> then we cannot accept this.  

> If you want to add something to your profile, e.g. fonts.dir, then use
> profile hooks for that.  See %default-profile-hooks in
> guix/profiles.scm.  However, we already have a hook to build fonts.dir,
> so I'm not sure what's missing here.

Yeah, that is the profile hook that is modified.

The reason is that there are multiple font directories that can also be shared 
by different font packages.

Currently, the hook to build fonts.dir only does it for the "truetype" 
directory.

What the patch does is merge the font trees and then call mkfontdir for each of 
the output directories (via ftw).

The modification in 'union-build is because it handles a special case of there 
just being one input directory: then 'union-build would have symlinked the 
entire directory - which would make the output (!) immutable.

In the end 'fonts-dir-file returns a derivation that is an union of the inputs 
with those fonts.dir files added.

That said, I'm not sure whether the added complexity is worth it - but it would 
be safe and it doesn't mutate existing store items in-place. It just adds 
"fonts.dir"s to the profile for all subdirs of share/fonts . It's avoiding 
symlinks in order to be able to do that without modifying the inputs in-place.

Re: [PATCH 1/2] build: union: Add create-all-directory? parameter to union-build

[宋文武]

"Huang, Ying"  writes:

Hi, sorry for my late reply.

> Danny Milosavljevic  writes:
>
>> Hi,
>>
>> yeah, I've looked at it and I see what you are trying to do.
>>
>> You modified 'union to replicate the directory tree and symlink only the 
>> non-directory files - in order to be able to add "fonts.dir" in those 
>> directories.
>>
>> The previous version symlinked entire directories, too, and you wouldn't 
>> have been able to add those "fonts.dir" files inside them later.
>>
>> Your fix seems kinda hacky - but I don't really see a downside.
>>
>> Thinking about it some more I think it looks good enough to me. I'd like a 
>> second opinion, though.
>>
>> Other than that, the name of the new flag "create-all-directory?" is weird. 
>> I'd like it to be something like "create-all-directories?".

>>
>> (The non-hacky solution would be to make 'union aware that there are
>> "fonts.dir" files to go in there, too. One could do that by adding
>> these as an input, too. I'm not sure whether that would be worth the
>> added complexity, though - especially since the fonts.dir generator
>> needs to have the finished union as input and not just single
>> directories one by one. Thoughts?)
>
> union will only be called with create-all-directory? != nil for packages
> have share/fonts directories.  And for these packages, we plan to remove
> the fonts.dir and fonts.scale files, so that we need to create all
> directories for them after all.  Is this good enough?
>

Yes, I thought so and it will work, but maybe it's better (non-hacky
way?) to make every font package install fonts.scale and fonts.dir files
into its own directory...

IIUC, to make them useful, we have to make the fonts directories
available to the running X server through xserver.conf (FontPath) or
adding them using 'xset +fp'.  And since either method is limited to a
single directory, so if we avoid the collision by putting each font
package under an unique path ($out/share/fonts/TYPE/FONT-NAME), then the
font-dir profile hook is not needed at all.

We can do this in the wanted (upcoming?) 'font-build-system'.

Re: [PATCH 1/2] build: union: Add create-all-directory? parameter to union-build

[Huang, Ying]

Mark H Weaver  writes:

> Danny Milosavljevic  writes:
>
>> yeah, I've looked at it and I see what you are trying to do.
>>
>> You modified 'union to replicate the directory tree and symlink only
>> the non-directory files - in order to be able to add "fonts.dir" in
>> those directories.
>
> If the purpose here is to facilitate mutating anything in the store,
> then we cannot accept this.  The entire design is Guix is based on store
> items being immutable, and we assume this throughout the code, most
> notably by assuming that store items and parts of store items can be
> aliased instead of copied.
>
> If you want to add something to your profile, e.g. fonts.dir, then use
> profile hooks for that.  See %default-profile-hooks in
> guix/profiles.scm.  However, we already have a hook to build fonts.dir,
> so I'm not sure what's missing here.

This is to be used by fonts-dir-file profile hook.  The 2/2 of the
patchset will enhance the fonts-dir-file profile hook based on mechanism
provided here.  The problem we want to resolve in this patchset is that
in addition to $GUIX_PROFILE/share/fonts/truetype, we need to create
fonts.dir/scale for other directories in $GUIX_PROFILE/share/fonts too.
So we need to create all directories under $GUIX_PROFILE/share/fonts.
We do that in 2/2 of the patchset.

Best Regards,
Huang, Ying

> If we've misunderstood the rationale for this patch, can you help us
> understand what you're trying to do?
>
>  Thanks,
>Mark

Re: [PATCH 1/2] build: union: Add create-all-directory? parameter to union-build

[Huang, Ying]

Danny Milosavljevic  writes:

> Hi,
>
> yeah, I've looked at it and I see what you are trying to do.
>
> You modified 'union to replicate the directory tree and symlink only the 
> non-directory files - in order to be able to add "fonts.dir" in those 
> directories.
>
> The previous version symlinked entire directories, too, and you wouldn't have 
> been able to add those "fonts.dir" files inside them later.
>
> Your fix seems kinda hacky - but I don't really see a downside.
>
> Thinking about it some more I think it looks good enough to me. I'd like a 
> second opinion, though.
>
> Other than that, the name of the new flag "create-all-directory?" is weird. 
> I'd like it to be something like "create-all-directories?".
>
> (The non-hacky solution would be to make 'union aware that there are
> "fonts.dir" files to go in there, too. One could do that by adding
> these as an input, too. I'm not sure whether that would be worth the
> added complexity, though - especially since the fonts.dir generator
> needs to have the finished union as input and not just single
> directories one by one. Thoughts?)

union will only be called with create-all-directory? != nil for packages
have share/fonts directories.  And for these packages, we plan to remove
the fonts.dir and fonts.scale files, so that we need to create all
directories for them after all.  Is this good enough?

Best Regards,
Huang, Ying

Re: [PATCH 1/2] build: union: Add create-all-directory? parameter to union-build

[Huang, Ying]

Danny Milosavljevic  writes:

> Hi,
>
> yeah, I've looked at it and I see what you are trying to do.
>
> You modified 'union to replicate the directory tree and symlink only the 
> non-directory files - in order to be able to add "fonts.dir" in those 
> directories.
>
> The previous version symlinked entire directories, too, and you wouldn't have 
> been able to add those "fonts.dir" files inside them later.
>
> Your fix seems kinda hacky - but I don't really see a downside.
>
> Thinking about it some more I think it looks good enough to me. I'd like a 
> second opinion, though.
>
> Other than that, the name of the new flag "create-all-directory?" is weird. 
> I'd like it to be something like "create-all-directories?".

OK.  Will change it.

Best Regards,
Huang, Ying

> (The non-hacky solution would be to make 'union aware that there are
> "fonts.dir" files to go in there, too. One could do that by adding
> these as an input, too. I'm not sure whether that would be worth the
> added complexity, though - especially since the fonts.dir generator
> needs to have the finished union as input and not just single
> directories one by one. Thoughts?)

Re: [PATCH 1/2] build: union: Add create-all-directory? parameter to union-build

[Huang, Ying]

Hi, Andy,

Andy Wingo  writes:

> Hi :)
>
> On Fri 24 Feb 2017 13:40, "Huang, Ying"  writes:
>
>>>(define (union output inputs)
>>>  (match inputs
>>>((input)
>>> -   ;; There's only one input, so just make a link.
>>> -   (symlink* input output))
>>> +   ;; There's only one input, so just make a link unless 
>>> create-all-directory?
>
> This comment should be a full sentence ending with a period, with line
> wrapped at 72 columns.

Sure.

>>> +   (if (and create-all-directory? (file-is-directory? input))
>>> +   (union-of-directories output inputs)
>>> +   (symlink* input output)))
>
> What's the use case?  Something that can handle some symlinks in the
> tree but not all symlinks in the tree?  I think this patch is a bad idea
> for union-build because it has a strange effect that's hard to describe
> up and down the union (both at the root and at branch directories).  I
> think it's probably a better idea to go for something more fit to your
> purpose and only pull it into union-build once you see yourself needing
> it more than once.  IMO anyway :)

The use case is the 2/2 of the patchset,

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

I want to create fonts.dir and fonts.scale for font packages, because
multiple font packages could install fonts into same directory.  This is
done for $GUIX_PROFILE/share/fonts/truetype only before, but now we want
to do that for all font directories in $GUIX_PROFILE/share/fonts.  Do
you have some idea for some better solution?

Best Regards,
Huang, Ying

>
> Andy

Re: [PATCH 1/2] build: union: Add create-all-directory? parameter to union-build

[Mark H Weaver]

Danny Milosavljevic  writes:

> yeah, I've looked at it and I see what you are trying to do.
>
> You modified 'union to replicate the directory tree and symlink only
> the non-directory files - in order to be able to add "fonts.dir" in
> those directories.

If the purpose here is to facilitate mutating anything in the store,
then we cannot accept this.  The entire design is Guix is based on store
items being immutable, and we assume this throughout the code, most
notably by assuming that store items and parts of store items can be
aliased instead of copied.

If you want to add something to your profile, e.g. fonts.dir, then use
profile hooks for that.  See %default-profile-hooks in
guix/profiles.scm.  However, we already have a hook to build fonts.dir,
so I'm not sure what's missing here.

If we've misunderstood the rationale for this patch, can you help us
understand what you're trying to do?

 Thanks,
   Mark

Re: [PATCH 1/2] build: union: Add create-all-directory? parameter to union-build

[Danny Milosavljevic]

Hi,

yeah, I've looked at it and I see what you are trying to do.

You modified 'union to replicate the directory tree and symlink only the 
non-directory files - in order to be able to add "fonts.dir" in those 
directories.

The previous version symlinked entire directories, too, and you wouldn't have 
been able to add those "fonts.dir" files inside them later.

Your fix seems kinda hacky - but I don't really see a downside.

Thinking about it some more I think it looks good enough to me. I'd like a 
second opinion, though.

Other than that, the name of the new flag "create-all-directory?" is weird. I'd 
like it to be something like "create-all-directories?".

(The non-hacky solution would be to make 'union aware that there are 
"fonts.dir" files to go in there, too. One could do that by adding these as an 
input, too. I'm not sure whether that would be worth the added complexity, 
though - especially since the fonts.dir generator needs to have the finished 
union as input and not just single directories one by one. Thoughts?)

Re: [PATCH 1/2] build: union: Add create-all-directory? parameter to union-build

[Andy Wingo]

Hi :)

On Fri 24 Feb 2017 13:40, "Huang, Ying"  writes:

>>(define (union output inputs)
>>  (match inputs
>>((input)
>> -   ;; There's only one input, so just make a link.
>> -   (symlink* input output))
>> +   ;; There's only one input, so just make a link unless 
>> create-all-directory?

This comment should be a full sentence ending with a period, with line
wrapped at 72 columns.

>> +   (if (and create-all-directory? (file-is-directory? input))
>> +   (union-of-directories output inputs)
>> +   (symlink* input output)))

What's the use case?  Something that can handle some symlinks in the
tree but not all symlinks in the tree?  I think this patch is a bad idea
for union-build because it has a strange effect that's hard to describe
up and down the union (both at the root and at branch directories).  I
think it's probably a better idea to go for something more fit to your
purpose and only pull it into union-build once you see yourself needing
it more than once.  IMO anyway :)

Andy

Re: [PATCH 1/2] build: union: Add create-all-directory? parameter to union-build

[Huang, Ying]

Hi, All,

Can anyone help me to review this patch?

Best Regards,
Huang, Ying

huang_ying_cari...@163.com writes:

> From: Huang Ying 
>
> * guix/build/union.scm (union-build): Add create-all-directory? keyword
>   parameter.  To add/remove some files from the directory.
> ---
>  guix/build/union.scm | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/guix/build/union.scm b/guix/build/union.scm
> index 6640b5652..4783066be 100644
> --- a/guix/build/union.scm
> +++ b/guix/build/union.scm
> @@ -73,7 +73,8 @@ identical, #f otherwise."
>(loop)
>  
>  (define* (union-build output inputs
> -  #:key (log-port (current-error-port)))
> +  #:key (log-port (current-error-port))
> +  (create-all-directory? #f))
>"Build in the OUTPUT directory a symlink tree that is the union of all
>  the INPUTS."
>  
> @@ -104,8 +105,10 @@ the INPUTS."
>(define (union output inputs)
>  (match inputs
>((input)
> -   ;; There's only one input, so just make a link.
> -   (symlink* input output))
> +   ;; There's only one input, so just make a link unless 
> create-all-directory?
> +   (if (and create-all-directory? (file-is-directory? input))
> +   (union-of-directories output inputs)
> +   (symlink* input output)))
>(_
> (call-with-values (lambda () (partition file-is-directory? inputs))
>   (match-lambda*