[OE-core] [PATCH 2/2] wic: Add --embed-rootfs argument

Wed Mar 4 14:31:55 UTC 2020

On 3/4/20 7:49 AM, Ricardo Ribalda Delgado wrote:
> Hi Paul
>
> On Wed, Mar 4, 2020 at 11:14 AM Ricardo Ribalda Delgado
> <ricardo at ribalda.com> wrote:
>> Hi Paul
>>
>> On Wed, Mar 4, 2020 at 11:09 AM Paul Barker <pbarker at konsulko.com> wrote:
>>> On Wed, 4 Mar 2020 10:56:20 +0100
>>> Ricardo Ribalda Delgado <ricardo at ribalda.com> wrote:
>>>
>>>> Hi Paul
>>>>
>>>> Thanks for your reply
>>>>
>>>> On Wed, Mar 4, 2020 at 10:43 AM Paul Barker <pbarker at konsulko.com> wrote:
>>>>> On Wed,  4 Mar 2020 09:34:38 +0100
>>>>> Ricardo Ribalda Delgado <ricardo at ribalda.com> wrote:
>>>>>
>>>>>> This option adds the content of a rootfs on a specific location on the
>>>>>> rootfs.
>>>>>>
>>>>>> It is very useful for making a partition that contains the rootfs for a
>>>>>> host and a target Eg:
>>>>>>
>>>>>> / -> Roofs for the host
>>>>>> /export/ -> Rootfs for the target (which will netboot)
>>>>>>
>>>>>> Although today we support making a partition for "/export" this might
>>>>>> not be compatible with some upgrade systems, or we might be limited by
>>>>>> the number of partitions.
>>>>>>
>>>>>> With this patch we can use something like:
>>>>>>
>>>>>> part / --source rootfs --embed-rootfs=target-image:/export
>>>>> I considered using a syntax like this for --include-path but I chose not to
>>>>> as it's not easy to choose a separator. ':' is a valid character in a
>>>>> directory name.
>>>> I think we have to live with not being able to copy files to a
>>>> directory with a colon :(
>>> I wouldn't like to place a limitation like that on users and I'd like to
>>> avoid the proliferation of similar arguments here.
>>>
>>> How about modifying '--include-path' to take two arguments, a source path and
>>> a destination prefix? The python argument parser should be able to handle
>>> this and you can build a list of tuples (source_path, dest_prefix) when
>>> parsing the arguments.
>>>
>>> The '--include-path' argument hasn't been part of a Yocto release yet it's
>>> just on the master branch so I think if we're going to fix it with a breaking
>>> change, now is definitely the time.
>> That sounds good to me. I do not mind having two arguments. for
>> embed-rootfs. and probably will look better on include-path
> How do you think that the parameters should look like:
>
> --embed-rootfs target-image /export target-image2 /export2
> --embed-rootfs (target-image, /export) (target-image2, /export2)
>   ??

FWIW, it should probably end up looking like this:

  --embed-rootfs target-image /export --embed-rootfs target-image2 /export2

The python argparse snippet you would use for this would be something like:

  parser.add_argument('--embed-rootfs', nargs=2, action='append', 
default=(), help='...')

>
> Shall I change it for include-path and embed-rootfs or you want to
> take care of include-path?
>
>>>>> My approach is to handle this in two steps:
>>>>>
>>>>> 1) Add a new task before do_image_wic which creates an 'embedded-rootfs'
>>>>>    directory and copies 'target-image' to 'embedded-rootfs/export'.
>>>>>
>>>>> 2) Use '--include-path=embedded-rootfs' in the wks file.
>>>>>
>>>> The biggest problem with that approach is permissions. We need files
>>>> with UID 0, siticky bits, etc. We lose the pseudo database if we
>>>> simply copy to a folder.
>>>>
>>>> Also, if we have a complex system with multiple partitions I prefer to
>>>> handle all in a single .wks file instead of a script + wks file
>>>>
>>>>>> on the .wks file.
>>>>>>
>>>>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo at ribalda.com>
>>>>>> ---
>>>>>>   scripts/lib/wic/help.py                  |  7 +++++++
>>>>>>   scripts/lib/wic/ksparser.py              |  1 +
>>>>>>   scripts/lib/wic/partition.py             |  1 +
>>>>>>   scripts/lib/wic/plugins/source/rootfs.py | 20 +++++++++++++++++++-
>>>>>>   4 files changed, 28 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/scripts/lib/wic/help.py b/scripts/lib/wic/help.py
>>>>>> index 4d342fcf05..67a33e6a65 100644
>>>>>> --- a/scripts/lib/wic/help.py
>>>>>> +++ b/scripts/lib/wic/help.py
>>>>>> @@ -979,6 +979,13 @@ DESCRIPTION
>>>>>>                            copies. This option only has an effect with the rootfs
>>>>>>                            source plugin.
>>>>>>
>>>>>> +         --embed-rootfs: This option is specific to wic. It embeds a rootfs into
>>>>>> +                         the given path to the resulting image. The option
>>>>>> +                         contains two fields, the roofs and the path, separated
>>>>>> +                         by a colon. The rootfs follows the same logic as the
>>>>>> +                         rootfs-dir argument. This option only has an effect
>>>>>> +                         with the rootfs source plugin.
>>>>>> +
>>>>>>            --extra-space: This option is specific to wic. It adds extra
>>>>>>                           space after the space filled by the content
>>>>>>                           of the partition. The final size can go
>>>>>> diff --git a/scripts/lib/wic/ksparser.py b/scripts/lib/wic/ksparser.py
>>>>>> index 650b976223..d422e2a6bb 100644
>>>>>> --- a/scripts/lib/wic/ksparser.py
>>>>>> +++ b/scripts/lib/wic/ksparser.py
>>>>>> @@ -138,6 +138,7 @@ class KickStart():
>>>>>>           part.add_argument('--align', type=int)
>>>>>>           part.add_argument('--exclude-path', nargs='+')
>>>>>>           part.add_argument('--include-path', nargs='+')
>>>>>> +        part.add_argument('--embed-rootfs', nargs='+')
>>>>>>           part.add_argument("--extra-space", type=sizetype)
>>>>>>           part.add_argument('--fsoptions', dest='fsopts')
>>>>>>           part.add_argument('--fstype', default='vfat',
>>>>>> diff --git a/scripts/lib/wic/partition.py b/scripts/lib/wic/partition.py
>>>>>> index 2d95f78439..13857df82f 100644
>>>>>> --- a/scripts/lib/wic/partition.py
>>>>>> +++ b/scripts/lib/wic/partition.py
>>>>>> @@ -31,6 +31,7 @@ class Partition():
>>>>>>           self.extra_space = args.extra_space
>>>>>>           self.exclude_path = args.exclude_path
>>>>>>           self.include_path = args.include_path
>>>>>> +        self.embed_rootfs = args.embed_rootfs
>>>>>>           self.fsopts = args.fsopts
>>>>>>           self.fstype = args.fstype
>>>>>>           self.label = args.label
>>>>>> diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py
>>>>>> index 40419a64b3..16359601dc 100644
>>>>>> --- a/scripts/lib/wic/plugins/source/rootfs.py
>>>>>> +++ b/scripts/lib/wic/plugins/source/rootfs.py
>>>>>> @@ -80,7 +80,7 @@ class RootfsPlugin(SourcePlugin):
>>>>>>
>>>>>>           new_rootfs = None
>>>>>>           # Handle excluded paths.
>>>>>> -        if part.exclude_path or part.include_path:
>>>>>> +        if part.exclude_path or part.include_path or part.embed_rootfs:
>>>>>>               # We need a new rootfs directory we can delete files from. Copy to
>>>>>>               # workdir.
>>>>>>               new_rootfs = os.path.realpath(os.path.join(cr_workdir, "rootfs%d" % part.lineno))
>>>>>> @@ -100,6 +100,24 @@ class RootfsPlugin(SourcePlugin):
>>>>>>               for path in part.include_path or []:
>>>>>>                   copyhardlinktree(path, new_rootfs)
>>>>>>
>>>>>> +            for embed in part.embed_rootfs or []:
>>>>>> +                [embed_rootfs, path] = embed.split(":")
>>>>>> +                #we need to remove the initial / for os.path.join to work
>>>>>> +                if os.path.isabs(path):
>>>>>> +                    path = path[1:]
>>>>>> +                if embed_rootfs in krootfs_dir:
>>>>>> +                    embed_rootfs = krootfs_dir[embed_rootfs]
>>>>>> +                embed_rootfs = cls.__get_rootfs_dir(embed_rootfs)
>>>>>> +                tar_file = os.path.realpath(os.path.join(cr_workdir, "aux.tar"))
>>>>>> +                tar_cmd = "%s tar cpf %s -C %s ." % (cls.__get_pseudo(native_sysroot,
>>>>>> +                                                     embed_rootfs), tar_file, embed_rootfs)
>>>>>> +                exec_native_cmd(tar_cmd, native_sysroot)
>>>>>> +                untar_cmd = "%s tar xf %s -C %s ." % (cls.__get_pseudo(native_sysroot, new_rootfs),
>>>>>> +                                                      tar_file, os.path.join(new_rootfs, path))
>>>>>> +                exec_native_cmd(untar_cmd, native_sysroot,
>>>>>> +                                cls.__get_pseudo(native_sysroot, new_rootfs))
>>>>>> +                os.remove(tar_file)
>>>>>> +
>>>>> Why are you using an intermediate tar file here?
>>>> For the permissions :(
>>> Your previous patch handled this using 'pseudo -B'. It would be good to be
>>> consistent if possible - either use an intermediate tar file in all cases if
>>> it's necessary or use 'pseudo -B' in all cases.
>> Unfortunately, you cannot use pseudo -B in this case.
>>
>> You use pseudo -B when you move one pseudo-aware partition to
>> somewhere else. But now we are merging two pseudo-aware partitions.
>>
>> Also I would not use the intermediate tar on the first place because
>> it is a waste of space + cpu resources.  Copyhardlink is the right
>> tool
>>
>>
>>> --
>>> Paul Barker
>>> Konsulko Group
>>
>>
>> --
>> Ricardo Ribalda
>
>