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

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

 1 files changed, 10 insertions(+), 3 deletions(-)

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

Thanks, merging!

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

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.

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

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

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)

Luca Weiss <luca@z3ntu.xyz>

1 year, 6 months ago

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)

Caleb Connolly <kc@postmarketos.org>

1 year, 6 months ago

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

Oliver Smith <ollieparanoid@postmarketos.org>

1 year, 6 months ago

Oliver Smith <ollieparanoid@postmarketos.org>

1 year, 6 months ago

Applied, thanks!

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

Best regards

builds.sr.ht <builds@sr.ht>

1 year, 6 months ago

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]: mailto:ablocorrea@hotmail.com

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