~postmarketos/pmbootstrap-devel

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
6 4

[PATCH pmbootstrap v4] install: write new file instead of modifying locale.sh from alpine-baselayout

Pablo Correa Gómez <ablocorrea@hotmail.com>
Details
Message ID
<DB9P192MB12912380DD72F8A840694B44C7759@DB9P192MB1291.EURP192.PROD.OUTLOOK.COM>
DKIM signature
missing
Download raw message
Patch: +10 -3
First of all, modifying in-place the file owned by alpine-baselayout has the
consequence of that file never being updated by APK. This is an issue changes
happen upstream. And I just fixed[1] an issue upstream that had to be with
that exact file, so make sure that from now on, we're writing to another file
that sorts before the one from alpine-baselayout. Additionally, equivalently
to the fix in [1] for bug [2], don't set the variable unconditionally, but
instead use its current value if it's already set.

[1] https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/46718
[2] https://gitlab.alpinelinux.org/alpine/aports/-/issues/14862

Signed-off-by: Pablo Correa Gómez <ablocorrea@hotmail.com>
---

v4:
 * Renamed file to 10locale-pmos.sh
 * Changed ownership of file to root

 pmb/install/_install.py | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/pmb/install/_install.py b/pmb/install/_install.py
index 216b8419..ba808627 100644
--- a/pmb/install/_install.py
+++ b/pmb/install/_install.py
@@ -1032,9 +1032,16 @@ def create_device_rootfs(args, step, steps):

    # Set locale
    if locale_is_set:
        pmb.chroot.root(args, ["sed", "-i",
                               f"s/LANG=C.UTF-8/LANG={args.locale}/",
                               "/etc/profile.d/locale.sh"], suffix)
        # 10locale-pmos.sh gets sourced before 20locale.sh from
        # alpine-baselayout by /etc/profile. Since they don't override the
        # locale if it exists, it warranties we have preference
        path = "/etc/profile.d/10locale-pmos.sh"
        path_tmp = "/tmp/locale.sh"
        path_outside = f"{args.work}/chroot_{suffix}{path_tmp}"
        with open(path_outside, "w", encoding="utf-8") as handle:
            handle.write(f"export LANG=${{LANG:-{args.locale}}}")
        pmb.chroot.root(args, ["mv", path_tmp, path], suffix)
        pmb.chroot.root(args, ["chown", "root:root", path], suffix)

    # Set the hostname as the device name
    setup_hostname(args)
--
2.40.1

[pmbootstrap/patches/.build.yml] build success

builds.sr.ht <builds@sr.ht>
Details
Message ID
<CSKKWFJM562C.3LOAD01LTKXMS@cirno2>
In-Reply-To
<DB9P192MB12912380DD72F8A840694B44C7759@DB9P192MB1291.EURP192.PROD.OUTLOOK.COM> (view parent)
DKIM signature
missing
Download raw message
pmbootstrap/patches/.build.yml: SUCCESS in 16m42s

[install: write new file instead of modifying locale.sh from alpine-baselayout][0] v4 from [Pablo Correa Gómez][1]

[0]: https://lists.sr.ht/~postmarketos/pmbootstrap-devel/patches/41092
[1]: ablocorrea@hotmail.com

✓ #988880 SUCCESS pmbootstrap/patches/.build.yml https://builds.sr.ht/~postmarketos/job/988880

Re: [PATCH pmbootstrap v4] install: write new file instead of modifying locale.sh from alpine-baselayout

Details
Message ID
<13369976.O9o76ZdvQC@z3ntu.xyz>
In-Reply-To
<DB9P192MB12912380DD72F8A840694B44C7759@DB9P192MB1291.EURP192.PROD.OUTLOOK.COM> (view parent)
DKIM signature
missing
Download raw message
On Freitag, 12. Mai 2023 22:07:09 CEST Pablo Correa Gómez wrote:
> First of all, modifying in-place the file owned by alpine-baselayout has the
> consequence of that file never being updated by APK. This is an issue
> changes happen upstream. And I just fixed[1] an issue upstream that had to
> be with that exact file, so make sure that from now on, we're writing to
> another file that sorts before the one from alpine-baselayout.
> Additionally, equivalently to the fix in [1] for bug [2], don't set the
> variable unconditionally, but instead use its current value if it's already
> set.
> 
> [1] https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/46718
> [2] https://gitlab.alpinelinux.org/alpine/aports/-/issues/14862
> 
> Signed-off-by: Pablo Correa Gómez <ablocorrea@hotmail.com>
> ---
> 
> v4:
>  * Renamed file to 10locale-pmos.sh
>  * Changed ownership of file to root
> 
>  pmb/install/_install.py | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/pmb/install/_install.py b/pmb/install/_install.py
> index 216b8419..ba808627 100644
> --- a/pmb/install/_install.py
> +++ b/pmb/install/_install.py
> @@ -1032,9 +1032,16 @@ def create_device_rootfs(args, step, steps):
> 
>      # Set locale
>      if locale_is_set:
> -        pmb.chroot.root(args, ["sed", "-i",
> -                               f"s/LANG=C.UTF-8/LANG={args.locale}/",
> -                               "/etc/profile.d/locale.sh"], suffix)
> +        # 10locale-pmos.sh gets sourced before 20locale.sh from
> +        # alpine-baselayout by /etc/profile. Since they don't override the
> +        # locale if it exists, it warranties we have preference
> +        path = "/etc/profile.d/10locale-pmos.sh"
> +        path_tmp = "/tmp/locale.sh"
> +        path_outside = f"{args.work}/chroot_{suffix}{path_tmp}"
> +        with open(path_outside, "w", encoding="utf-8") as handle:
> +            handle.write(f"export LANG=${{LANG:-{args.locale}}}")
> +        pmb.chroot.root(args, ["mv", path_tmp, path], suffix)
> +        pmb.chroot.root(args, ["chown", "root:root", path], suffix)

I don't want to drag this review process longer than it needs to be but why do
this dance of writing to /tmp and then moving & chown'ing it and not do
something like this which sounds simpler?

    # Generate /etc/hostname
    pmb.chroot.root(args, ["sh", "-c", "echo " + shlex.quote(hostname) +
                           " > /etc/hostname"], suffix)

So for this case I guess this:

    pmb.chroot.root(args, ["sh", "-c", "echo " + f"export LANG=${{LANG:-{args.locale}}}" +
                           " > /etc/profile.d/10locale-pmos.sh"], suffix)

Should be like half the lines of code it's right now?

In any case, thanks for handling this, definitely a good improvement (and
fix to make pmbootstrap install work again ;) )

Regards
Luca


> 
>      # Set the hostname as the device name
>      setup_hostname(args)
> --
> 2.40.1
Details
Message ID
<d9a750ea-1f09-f7f1-a917-5e88976cb588@postmarketos.org>
In-Reply-To
<13369976.O9o76ZdvQC@z3ntu.xyz> (view parent)
DKIM signature
missing
Download raw message

On 13/05/2023 09:11, Luca Weiss wrote:
> On Freitag, 12. Mai 2023 22:07:09 CEST Pablo Correa Gómez wrote:
>> First of all, modifying in-place the file owned by alpine-baselayout has the
>> consequence of that file never being updated by APK. This is an issue
>> changes happen upstream. And I just fixed[1] an issue upstream that had to
>> be with that exact file, so make sure that from now on, we're writing to
>> another file that sorts before the one from alpine-baselayout.
>> Additionally, equivalently to the fix in [1] for bug [2], don't set the
>> variable unconditionally, but instead use its current value if it's already
>> set.
>>
>> [1] https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/46718
>> [2] https://gitlab.alpinelinux.org/alpine/aports/-/issues/14862
>>
>> Signed-off-by: Pablo Correa Gómez <ablocorrea@hotmail.com>
>> ---
>>
>> v4:
>>  * Renamed file to 10locale-pmos.sh
>>  * Changed ownership of file to root
>>
>>  pmb/install/_install.py | 13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/pmb/install/_install.py b/pmb/install/_install.py
>> index 216b8419..ba808627 100644
>> --- a/pmb/install/_install.py
>> +++ b/pmb/install/_install.py
>> @@ -1032,9 +1032,16 @@ def create_device_rootfs(args, step, steps):
>>
>>      # Set locale
>>      if locale_is_set:
>> -        pmb.chroot.root(args, ["sed", "-i",
>> -                               f"s/LANG=C.UTF-8/LANG={args.locale}/",
>> -                               "/etc/profile.d/locale.sh"], suffix)
>> +        # 10locale-pmos.sh gets sourced before 20locale.sh from
>> +        # alpine-baselayout by /etc/profile. Since they don't override the
>> +        # locale if it exists, it warranties we have preference
>> +        path = "/etc/profile.d/10locale-pmos.sh"
>> +        path_tmp = "/tmp/locale.sh"
>> +        path_outside = f"{args.work}/chroot_{suffix}{path_tmp}"
>> +        with open(path_outside, "w", encoding="utf-8") as handle:
>> +            handle.write(f"export LANG=${{LANG:-{args.locale}}}")
>> +        pmb.chroot.root(args, ["mv", path_tmp, path], suffix)
>> +        pmb.chroot.root(args, ["chown", "root:root", path], suffix)
> 
> I don't want to drag this review process longer than it needs to be but why do
> this dance of writing to /tmp and then moving & chown'ing it and not do
> something like this which sounds simpler?
> 
>     # Generate /etc/hostname
>     pmb.chroot.root(args, ["sh", "-c", "echo " + shlex.quote(hostname) +
>                            " > /etc/hostname"], suffix)

As far as I know this doesn't work. I've messed with this kinda stuff in
pmbootstrap fairly recently and piping to a file is a no-go.
> 
> So for this case I guess this:
> 
>     pmb.chroot.root(args, ["sh", "-c", "echo " + f"export LANG=${{LANG:-{args.locale}}}" +
>                            " > /etc/profile.d/10locale-pmos.sh"], suffix)
> 
> Should be like half the lines of code it's right now?
> 
> In any case, thanks for handling this, definitely a good improvement (and
> fix to make pmbootstrap install work again ;) )
> 
> Regards
> Luca
> 
> 
>>
>>      # Set the hostname as the device name
>>      setup_hostname(args)
>> --
>> 2.40.1
> 
> 
> 
> 

-- 
Kind Regards,
Caleb (they/them)

Re: [PATCH pmbootstrap v4] install: write new file instead of modifying locale.sh from alpine-baselayout

Details
Message ID
<7096780.lOV4Wx5bFT@z3ntu.xyz>
In-Reply-To
<d9a750ea-1f09-f7f1-a917-5e88976cb588@postmarketos.org> (view parent)
DKIM signature
missing
Download raw message
On Samstag, 13. Mai 2023 11:47:11 CEST Caleb Connolly wrote:
> On 13/05/2023 09:11, Luca Weiss wrote:
> > On Freitag, 12. Mai 2023 22:07:09 CEST Pablo Correa Gómez wrote:
> >> First of all, modifying in-place the file owned by alpine-baselayout has
> >> the consequence of that file never being updated by APK. This is an
> >> issue changes happen upstream. And I just fixed[1] an issue upstream
> >> that had to be with that exact file, so make sure that from now on,
> >> we're writing to another file that sorts before the one from
> >> alpine-baselayout.
> >> Additionally, equivalently to the fix in [1] for bug [2], don't set the
> >> variable unconditionally, but instead use its current value if it's
> >> already
> >> set.
> >> 
> >> [1] https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/46718
> >> [2] https://gitlab.alpinelinux.org/alpine/aports/-/issues/14862
> >> 
> >> Signed-off-by: Pablo Correa Gómez <ablocorrea@hotmail.com>
> >> ---
> >> 
> >> v4:
> >>  * Renamed file to 10locale-pmos.sh
> >>  * Changed ownership of file to root
> >>  
> >>  pmb/install/_install.py | 13 ++++++++++---
> >>  1 file changed, 10 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/pmb/install/_install.py b/pmb/install/_install.py
> >> index 216b8419..ba808627 100644
> >> --- a/pmb/install/_install.py
> >> +++ b/pmb/install/_install.py
> >> 
> >> @@ -1032,9 +1032,16 @@ def create_device_rootfs(args, step, steps):
> >>      # Set locale
> >> 
> >>      if locale_is_set:
> >> -        pmb.chroot.root(args, ["sed", "-i",
> >> -                               f"s/LANG=C.UTF-8/LANG={args.locale}/",
> >> -                               "/etc/profile.d/locale.sh"], suffix)
> >> +        # 10locale-pmos.sh gets sourced before 20locale.sh from
> >> +        # alpine-baselayout by /etc/profile. Since they don't override
> >> the
> >> +        # locale if it exists, it warranties we have preference
> >> +        path = "/etc/profile.d/10locale-pmos.sh"
> >> +        path_tmp = "/tmp/locale.sh"
> >> +        path_outside = f"{args.work}/chroot_{suffix}{path_tmp}"
> >> +        with open(path_outside, "w", encoding="utf-8") as handle:
> >> +            handle.write(f"export LANG=${{LANG:-{args.locale}}}")
> >> +        pmb.chroot.root(args, ["mv", path_tmp, path], suffix)
> >> +        pmb.chroot.root(args, ["chown", "root:root", path], suffix)
> > 
> > I don't want to drag this review process longer than it needs to be but
> > why do this dance of writing to /tmp and then moving & chown'ing it and
> > not do something like this which sounds simpler?
> > 
> >     # Generate /etc/hostname
> >     pmb.chroot.root(args, ["sh", "-c", "echo " + shlex.quote(hostname) +
> >     
> >                            " > /etc/hostname"], suffix)
> 
> As far as I know this doesn't work. I've messed with this kinda stuff in
> pmbootstrap fairly recently and piping to a file is a no-go.

I've copied this hostname block from existing pmbootstrap code so apparently 
it does work ;)

> 
> > So for this case I guess this:
> >     pmb.chroot.root(args, ["sh", "-c", "echo " + f"export
> >     LANG=${{LANG:-{args.locale}}}" +>     
> >                            " > /etc/profile.d/10locale-pmos.sh"], suffix)
> > 
> > Should be like half the lines of code it's right now?
> > 
> > In any case, thanks for handling this, definitely a good improvement (and
> > fix to make pmbootstrap install work again ;) )
> > 
> > Regards
> > Luca
> > 
> >>      # Set the hostname as the device name
> >>      setup_hostname(args)
> >> 
> >> --
> >> 2.40.1
Details
Message ID
<CSMMVF8SN8KP.2HESJ7X46SPIV@pm-mail-aerc>
In-Reply-To
<7096780.lOV4Wx5bFT@z3ntu.xyz> (view parent)
DKIM signature
missing
Download raw message
On Sat May 13, 2023 at 11:59 AM CEST, Luca Weiss wrote:
> On Samstag, 13. Mai 2023 11:47:11 CEST Caleb Connolly wrote:
> > On 13/05/2023 09:11, Luca Weiss wrote:
> > > On Freitag, 12. Mai 2023 22:07:09 CEST Pablo Correa Gómez wrote:
> > >> First of all, modifying in-place the file owned by alpine-baselayout has
> > >> the consequence of that file never being updated by APK. This is an
> > >> issue changes happen upstream. And I just fixed[1] an issue upstream
> > >> that had to be with that exact file, so make sure that from now on,
> > >> we're writing to another file that sorts before the one from
> > >> alpine-baselayout.
> > >> Additionally, equivalently to the fix in [1] for bug [2], don't set the
> > >> variable unconditionally, but instead use its current value if it's
> > >> already
> > >> set.
> > >> 
> > >> [1] https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/46718
> > >> [2] https://gitlab.alpinelinux.org/alpine/aports/-/issues/14862
> > >> 
> > >> Signed-off-by: Pablo Correa Gómez <ablocorrea@hotmail.com>

Thanks, merging!

Reviewed-by: Oliver Smith <ollieparanoid@postmarketos.org>
Tested-by: Oliver Smith <ollieparanoid@postmarketos.org>

> > >> ---
> > >> 
> > >> v4:
> > >>  * Renamed file to 10locale-pmos.sh
> > >>  * Changed ownership of file to root
> > >>  
> > >>  pmb/install/_install.py | 13 ++++++++++---
> > >>  1 file changed, 10 insertions(+), 3 deletions(-)
> > >> 
> > >> diff --git a/pmb/install/_install.py b/pmb/install/_install.py
> > >> index 216b8419..ba808627 100644
> > >> --- a/pmb/install/_install.py
> > >> +++ b/pmb/install/_install.py
> > >> 
> > >> @@ -1032,9 +1032,16 @@ def create_device_rootfs(args, step, steps):
> > >>      # Set locale
> > >> 
> > >>      if locale_is_set:
> > >> -        pmb.chroot.root(args, ["sed", "-i",
> > >> -                               f"s/LANG=C.UTF-8/LANG={args.locale}/",
> > >> -                               "/etc/profile.d/locale.sh"], suffix)
> > >> +        # 10locale-pmos.sh gets sourced before 20locale.sh from
> > >> +        # alpine-baselayout by /etc/profile. Since they don't override
> > >> the
> > >> +        # locale if it exists, it warranties we have preference
> > >> +        path = "/etc/profile.d/10locale-pmos.sh"
> > >> +        path_tmp = "/tmp/locale.sh"
> > >> +        path_outside = f"{args.work}/chroot_{suffix}{path_tmp}"
> > >> +        with open(path_outside, "w", encoding="utf-8") as handle:
> > >> +            handle.write(f"export LANG=${{LANG:-{args.locale}}}")
> > >> +        pmb.chroot.root(args, ["mv", path_tmp, path], suffix)
> > >> +        pmb.chroot.root(args, ["chown", "root:root", path], suffix)
> > > 
> > > I don't want to drag this review process longer than it needs to be but
> > > why do this dance of writing to /tmp and then moving & chown'ing it and
> > > not do something like this which sounds simpler?

Right, we can do that, I'll adjust it before merging. Note that your
example was missing shlex.quote, which is important to escape this
properly and to not have $ interpreted by the sh command.

> > > 
> > >     # Generate /etc/hostname
> > >     pmb.chroot.root(args, ["sh", "-c", "echo " + shlex.quote(hostname) +
> > >     
> > >                            " > /etc/hostname"], suffix)
> > 
> > As far as I know this doesn't work. I've messed with this kinda stuff in
> > pmbootstrap fairly recently and piping to a file is a no-go.

It works, but it's a bit tricky to get right. I've updated the related
section of the wiki to mention this shorter method and to note that we
need to chown it with the method for longer files:

https://wiki.postmarketos.org/wiki/Pmbootstrap_development_guide#Writing_files_to_the_chroot

>
> I've copied this hostname block from existing pmbootstrap code so apparently 
> it does work ;)
>
> > 
> > > So for this case I guess this:
> > >     pmb.chroot.root(args, ["sh", "-c", "echo " + f"export
> > >     LANG=${{LANG:-{args.locale}}}" +>     
> > >                            " > /etc/profile.d/10locale-pmos.sh"], suffix)
> > > 
> > > Should be like half the lines of code it's right now?
> > > 
> > > In any case, thanks for handling this, definitely a good improvement (and
> > > fix to make pmbootstrap install work again ;) )
> > > 
> > > Regards
> > > Luca
> > > 
> > >>      # Set the hostname as the device name
> > >>      setup_hostname(args)
> > >> 
> > >> --
> > >> 2.40.1
Details
Message ID
<168413261999.6151.17813319713583080438.b4-ty@postmarketos.org>
In-Reply-To
<DB9P192MB12912380DD72F8A840694B44C7759@DB9P192MB1291.EURP192.PROD.OUTLOOK.COM> (view parent)
DKIM signature
missing
Download raw message
On Fri, 12 May 2023 22:07:09 +0200, Pablo Correa Gómez wrote:
> First of all, modifying in-place the file owned by alpine-baselayout has the
> consequence of that file never being updated by APK. This is an issue changes
> happen upstream. And I just fixed[1] an issue upstream that had to be with
> that exact file, so make sure that from now on, we're writing to another file
> that sorts before the one from alpine-baselayout. Additionally, equivalently
> to the fix in [1] for bug [2], don't set the variable unconditionally, but
> instead use its current value if it's already set.
> 
> [...]

Applied, thanks!

[1/1] install: write new file instead of modifying locale.sh from alpine-baselayout
      commit: acb94beaf98d038b19940cb49d2f6be68e006b56

Best regards,
-- 
Oliver Smith <ollieparanoid@postmarketos.org>
Reply to thread Export thread (mbox)