Hi Paul Thanks for your review, It has been already merged, but if there is something wrong we can send a patch fixing it.
https://git.openembedded.org/openembedded-core/commit/?id=36993eea89d1c011397b7692b9b8d61b499d0171 On Tue, Apr 7, 2020 at 8:13 PM Paul Barker <[email protected]> wrote: > > On Fri, 3 Apr 2020 21:53:39 +0200 > Ricardo Ribalda Delgado <[email protected]> wrote: > > > ping? > > I think that '../pseudo' should not be used here. I'll explain inline... > > > > > > > This results in a rootfs owned by the user that is running the wic > > > command (usually UID 1000), which makes some rootfs unbootable. > > > > > > To fix this we copy the content of the pseudo folders to the new folder > > > and modify the pseudo database using the "pseudo -B" command. > > > > > > Signed-off-by: Ricardo Ribalda Delgado <[email protected]> > > > --- > > > scripts/lib/wic/plugins/source/rootfs.py | 22 +++++++++++++++++++--- > > > 1 file changed, 19 insertions(+), 3 deletions(-) > > > > > > diff --git a/scripts/lib/wic/plugins/source/rootfs.py > > > b/scripts/lib/wic/plugins/source/rootfs.py > > > index 705aeb5563..40419a64b3 100644 > > > --- a/scripts/lib/wic/plugins/source/rootfs.py > > > +++ b/scripts/lib/wic/plugins/source/rootfs.py > > > @@ -16,11 +16,11 @@ import os > > > import shutil > > > import sys > > > > > > -from oe.path import copyhardlinktree > > > +from oe.path import copyhardlinktree, copytree > > > > > > from wic import WicError > > > from wic.pluginbase import SourcePlugin > > > -from wic.misc import get_bitbake_var > > > +from wic.misc import get_bitbake_var, exec_native_cmd > > > > > > logger = logging.getLogger('wic') > > > > > > @@ -44,6 +44,15 @@ class RootfsPlugin(SourcePlugin): > > > > > > return os.path.realpath(image_rootfs_dir) > > > > > > + @staticmethod > > > + def __get_pseudo(native_sysroot, rootfs): > > > + pseudo = "export PSEUDO_PREFIX=%s/usr;" % native_sysroot > > > + pseudo += "export PSEUDO_LOCALSTATEDIR=%s;" % > > > os.path.join(rootfs, "../pseudo") > > > + pseudo += "export PSEUDO_PASSWD=%s;" % rootfs > > > + pseudo += "export PSEUDO_NOSYMLINKEXP=1;" > > > + pseudo += "%s " % get_bitbake_var("FAKEROOTCMD") > > > + return pseudo > > > + > > > @classmethod > > > def do_prepare_partition(cls, part, source_params, cr, cr_workdir, > > > oe_builddir, bootimg_dir, kernel_dir, > > > @@ -78,9 +87,16 @@ class RootfsPlugin(SourcePlugin): > > > > > > if os.path.lexists(new_rootfs): > > > shutil.rmtree(os.path.join(new_rootfs)) > > > - > > > copyhardlinktree(part.rootfs_dir, new_rootfs) > > > > > > + if os.path.lexists(os.path.join(new_rootfs, "../pseudo")): > > new_rootfs is set by the following statement a few lines above: > > new_rootfs = os.path.realpath(os.path.join(cr_workdir, "rootfs%d" % > part.lineno)) > > Consider that `cr_workdir` may contain multiple rootfs staging directories > corresponding to multiple lines in the wks file, for example if a rootfs > image is duplicated into multiple partitions for redundancy. In that case > `os.path.join(new_rootfs, "../pseudo")` will clash between these different > rootfs copies. > > Let's use an explicit path instead, such as: > > new_pseudo = os.path.realpath(os.path.join(cr_workdir, "pseudo%d" % > part.lineno)) The reason to have that path was to follow the same structure as the real image.bb. If there are multiple partitions on the .wic file the different partitions are done one by one, not in parallel. So ../pseudo will be created for partition1 then it will be used to generate the partition1 ../pseudo will be deleted ../pseudo will be created for partition2 Even if they use the same partition, the code works (and ../pseudo is useless once the partition is generated) > > > > + shutil.rmtree(os.path.join(new_rootfs, "../pseudo")) > > > + copytree(os.path.join(part.rootfs_dir, "../pseudo"), > > part.rootfs_dir is whatever is given as the option to `--rootfs-dir`. There > is no guarantee that `../psuedo` is valid or if it corresponds to the rootfs > directory given. It's unsafe to step up the directory tree and make > assumptions like this. I think that if we do not pass a real rootfs to the rootfs plugin it is an error from the user. We can add a more beautiful error message instead of a backtrace. Or if you believe that it is a valid usecase to not pass a rootfs then we can continue with a warning/debug message and explicitly telling the user that the permissions are going to be invalid, because what he is using as a roofs is an unknow directory for bitbake. I have no personal preference for any of the two, tell me what do you prefer (or a different option) and I will implement it. Thans again for the review. > > > > + os.path.join(new_rootfs, "../pseudo")) > > > + pseudo_cmd = "%s -B -m %s -M %s" % > > > (cls.__get_pseudo(native_sysroot,new_rootfs), > > > + part.rootfs_dir, > > > new_rootfs) > > > + exec_native_cmd(pseudo_cmd, native_sysroot) > > > + > > > for path in part.include_path or []: > > > copyhardlinktree(path, new_rootfs) > > -- > Paul Barker > Konsulko Group > -- Ricardo Ribalda
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#137098): https://lists.openembedded.org/g/openembedded-core/message/137098 Mute This Topic: https://lists.openembedded.org/mt/72395284/21656 Group Owner: [email protected] Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
