diff mbox series

[bug#49082] guix: gexp: Make UTF-8 a default port encoding in mixed-text-file

Message ID 87bl836bdy.fsf@trop.in
State Accepted
Headers show
Series [bug#49082] guix: gexp: Make UTF-8 a default port encoding in mixed-text-file | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/issue success View issue

Commit Message

Andrew Tropin June 18, 2021, 4:15 a.m. UTC
guix/gexp.scm (mixed-text-file): It's necessary because if some UTF-8 symbols
appear in configuration, resulting file in the store will contain ? instead of
it.
---
 guix/gexp.scm | 1 +
 1 file changed, 1 insertion(+)

Comments

Leo Prikler June 18, 2021, 7:55 a.m. UTC | #1
I appreciate this change, but how many rebuilds will it cause? It might
need to go to core-updates.
Regards, Leo
Andrew Tropin June 18, 2021, 4:21 p.m. UTC | #2
Leo Prikler <leo.prikler@student.tugraz.at> writes:

> I appreciate this change, but how many rebuilds will it cause? It might
> need to go to core-updates.
> Regards, Leo

Not so many I suppose, AFAIK, it's not used by any packages, so it will
cause rebuild only for users of 2 dozens of system services during
system reconfigure, which should be quite cheap, because it's just
string concatenation.  Let me know if my understanding is wrong here.

Anyway, I don't mind it going to core-updates if you think it's
necessary.
Leo Prikler June 18, 2021, 4:39 p.m. UTC | #3
Am Freitag, den 18.06.2021, 19:21 +0300 schrieb Andrew Tropin:
> Leo Prikler <leo.prikler@student.tugraz.at> writes:
> 
> > I appreciate this change, but how many rebuilds will it cause? It
> > might
> > need to go to core-updates.
> > Regards, Leo
> 
> Not so many I suppose, AFAIK, it's not used by any packages, so it
> will
> cause rebuild only for users of 2 dozens of system services during
> system reconfigure, which should be quite cheap, because it's just
> string concatenation.  Let me know if my understanding is wrong here.
> 
> Anyway, I don't mind it going to core-updates if you think it's
> necessary.
In that case it's probably good to go to master.
One thing to note however, is that we typically bind %default-port-
encoding throughout Guix.  I suppose this doesn't work for stuff like
mixed-text-file, since it's executed in a different context?
Andrew Tropin June 18, 2021, 5:21 p.m. UTC | #4
Leo Prikler <leo.prikler@student.tugraz.at> writes:

> Am Freitag, den 18.06.2021, 19:21 +0300 schrieb Andrew Tropin:
>> Leo Prikler <leo.prikler@student.tugraz.at> writes:
>> 
>> > I appreciate this change, but how many rebuilds will it cause? It
>> > might
>> > need to go to core-updates.
>> > Regards, Leo
>> 
>> Not so many I suppose, AFAIK, it's not used by any packages, so it
>> will
>> cause rebuild only for users of 2 dozens of system services during
>> system reconfigure, which should be quite cheap, because it's just
>> string concatenation.  Let me know if my understanding is wrong here.
>> 
>> Anyway, I don't mind it going to core-updates if you think it's
>> necessary.
> In that case it's probably good to go to master.
Good!
> One thing to note however, is that we typically bind %default-port-
> encoding throughout Guix.  I suppose this doesn't work for stuff like
> mixed-text-file, since it's executed in a different context?
Very probably.

Have a good weekend!
Ludovic Courtès June 23, 2021, 9:39 p.m. UTC | #5
Hi,

Andrew Tropin <andrew@trop.in> skribis:

> guix/gexp.scm (mixed-text-file): It's necessary because if some UTF-8 symbols
> appear in configuration, resulting file in the store will contain ? instead of
> it.

Good catch!  Applied as 1f3d7b45349d43e5cc02594083e0cd44ef730992.

Please look how I modified the commit log, in line with our conventions
and previous entries for this file.

>    (define build
>      (gexp (call-with-output-file (ungexp output "out")
>              (lambda (port)
> +              (set-port-encoding! port "UTF-8")

Leo is right that we’d often write:

  (parameterize ((%default-port-encoding "UTF-8"))
    (call-with-output-file …))

because it’s marginally cleaner (the port is created with the right
encoding from the start).  In this case it doesn’t matter much though.

Thank you!

Ludo’.
Andrew Tropin June 24, 2021, 4:41 a.m. UTC | #6
Ludovic Courtès <ludo@gnu.org> writes:

> Hi,
>
> Andrew Tropin <andrew@trop.in> skribis:
>
>> guix/gexp.scm (mixed-text-file): It's necessary because if some UTF-8 symbols
>> appear in configuration, resulting file in the store will contain ? instead of
>> it.
>
> Good catch!  Applied as 1f3d7b45349d43e5cc02594083e0cd44ef730992.

Thank you very much!)

> Please look how I modified the commit log, in line with our conventions
> and previous entries for this file.
>
>>    (define build
>>      (gexp (call-with-output-file (ungexp output "out")
>>              (lambda (port)
>> +              (set-port-encoding! port "UTF-8")
>
> Leo is right that we’d often write:
>
>   (parameterize ((%default-port-encoding "UTF-8"))
>     (call-with-output-file …))
>
> because it’s marginally cleaner (the port is created with the right
> encoding from the start).  In this case it doesn’t matter much though.

Got it, I thought Leo meant to wrap (mixed-text-file ...) into
parametrize, which probably won't work, because gexp will be avaluated
in different environment.  Sorry for missunderstanding!)  Leo, Ludo,
thank you for explanation!

> Thank you!
>
> Ludo’.
diff mbox series

Patch

diff --git a/guix/gexp.scm b/guix/gexp.scm
index afb935761e..187f5c5e85 100644
--- a/guix/gexp.scm
+++ b/guix/gexp.scm
@@ -1921,6 +1921,7 @@  This is the declarative counterpart of 'text-file*'."
   (define build
     (gexp (call-with-output-file (ungexp output "out")
             (lambda (port)
+              (set-port-encoding! port "UTF-8")
               (display (string-append (ungexp-splicing text)) port)))))
 
   (computed-file name build))