[OE-core] [PATCH v7] do_image: Implement IMAGE_ROOTFS_EXCLUDE_PATH feature.

[Kristian Amlie]

On 25/01/18 11:58, Martin Hundebøll wrote:
> Hi Kristian,
> 
> Thanks for your work on this!
> 
> On 2018-01-25 11:33, Kristian Amlie wrote:
>> This is a direct followup from the earlier 6602392db3d39 commit in
>> wic. It works more or less the same way: The variable specifies a list
>> of directories relative to the root of the rootfs, and these
>> directories will be excluded from the resulting rootfs image. If an
>> entry ends with a slash, only the contents are omitted, not the
>> directory itself.
>>
>> Since the intended use of the variable is to exclude certain
>> directories from the rootfs, and then include said directories in
>> other partitions, it is not natural for this variable to be respected
>> for image creators that create multi partition images. These can turn
>> the feature off locally by defining:
>>
>>    do_image_myfs[respect_exclude_path] = "0"
>>
>> Specifically, "wic" and "multiubi" come with the feature disabled.
>>
>> Signed-off-by: Kristian Amlie <kristian.amlie at northern.tech>
>> ---
>>   meta/classes/image.bbclass           | 84
>> +++++++++++++++++++++++++++++++++++-
>>   meta/classes/image_types.bbclass     |  1 +
>>   meta/classes/image_types_wic.bbclass |  1 +
>>   3 files changed, 84 insertions(+), 2 deletions(-)
>>
>> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
>> index 4531aa2..849a19c 100644
>> --- a/meta/classes/image.bbclass
>> +++ b/meta/classes/image.bbclass
>> @@ -117,7 +117,8 @@ def rootfs_variables(d):
>>                   
>> 'IMAGE_ROOTFS_MAXSIZE','IMAGE_NAME','IMAGE_LINK_NAME','IMAGE_MANIFEST','DEPLOY_DIR_IMAGE','IMAGE_FSTYPES','IMAGE_INSTALL_COMPLEMENTARY','IMAGE_LINGUAS',
>>
>>                   
>> 'MULTILIBRE_ALLOW_REP','MULTILIB_TEMP_ROOTFS','MULTILIB_VARIANTS','MULTILIBS','ALL_MULTILIB_PACKAGE_ARCHS','MULTILIB_GLOBAL_VARIANTS','BAD_RECOMMENDATIONS','NO_RECOMMENDATIONS',
>>
>>                   
>> 'PACKAGE_ARCHS','PACKAGE_CLASSES','TARGET_VENDOR','TARGET_ARCH','TARGET_OS','OVERRIDES','BBEXTENDVARIANT','FEED_DEPLOYDIR_BASE_URI','INTERCEPT_DIR','USE_DEVFS',
>>
>> -                 'CONVERSIONTYPES', 'IMAGE_GEN_DEBUGFS',
>> 'ROOTFS_RO_UNNEEDED', 'IMGDEPLOYDIR', 'PACKAGE_EXCLUDE_COMPLEMENTARY',
>> 'REPRODUCIBLE_TIMESTAMP_ROOTFS']
>> +                 'CONVERSIONTYPES', 'IMAGE_GEN_DEBUGFS',
>> 'ROOTFS_RO_UNNEEDED', 'IMGDEPLOYDIR', 'PACKAGE_EXCLUDE_COMPLEMENTARY',
>> 'REPRODUCIBLE_TIMESTAMP_ROOTFS',
>> +                 'IMAGE_ROOTFS_EXCLUDE_PATH']
>>       variables.extend(rootfs_command_variables(d))
>>       variables.extend(variable_depends(d))
>>       return " ".join(variables)
>> @@ -508,8 +509,9 @@ python () {
>>           d.setVarFlag(task, 'func', '1')
>>           d.setVarFlag(task, 'fakeroot', '1')
>>   -        d.appendVarFlag(task, 'prefuncs', ' ' + debug + '
>> set_image_size')
>> +        d.appendVarFlag(task, 'prefuncs', ' ' + debug + '
>> set_image_size prepare_excluded_directories')
>>           d.prependVarFlag(task, 'postfuncs', ' create_symlinks')
>> +        d.appendVarFlag(task, 'postfuncs', '
>> cleanup_excluded_directories')
>>           d.appendVarFlag(task, 'subimages', ' ' + ' '.join(subimages))
>>           d.appendVarFlag(task, 'vardeps', ' ' + ' '.join(vardeps))
>>           d.appendVarFlag(task, 'vardepsexclude', 'DATETIME DATE ' + '
>> '.join(vardepsexclude))
>> @@ -518,6 +520,84 @@ python () {
>>           bb.build.addtask(task, 'do_image_complete', after, d)
>>   }
>>   +python prepare_excluded_directories() {
>> +    exclude_var = d.getVar('IMAGE_ROOTFS_EXCLUDE_PATH')
>> +    if not exclude_var:
>> +        return
>> +
>> +    taskname = d.getVar("BB_CURRENTTASK")
>> +
>> +    if d.getVarFlag('do_%s' % taskname, 'respect_exclude_path') == '0':
>> +        bb.debug(1, "'IMAGE_ROOTFS_EXCLUDE_PATH' is set but
>> 'respect_exclude_path' variable flag is 0 for this image type, so
>> ignoring it")
>> +        return
> 
> If I understand this correctly, paths like "/data" aren't allowed in
> "IMAGE_ROOTFS_EXCLUDE_PATH". That seems counter-intuitive to me, as I
> would expect this feature to take absolute paths inside the target image
> to be excluded.

My initial idea was absolute paths, but other maintainers disagreed
[1]. I don't think it makes sense to change this now, because it has
already been included in wic for a long time, and this patch is just
mirroring wic's behavior.

[1] http://lists.openembedded.org/pipermail/openembedded-core/2016-November/129301.html

> If the check is there to avoid "os.path.join()" returning the RHS only
> instead of the joined path, then please use "oe.path.join()" instead.

Oh, well spotted, I didn't know about oe.path.join(). Strictly speaking
it is not necessary since we require a relative path, but it is an extra
layer of safety, so yeah, I'll change it to use oe.path.join().

I also noticed there was an unneeded os.path.join() at the rmtree (it
joined one single string only), so I removed that one.

Rebased patch included with the changes.

>> +    import shutil
>> +    from oe.path import copyhardlinktree
>> +
>> +    exclude_list = exclude_var.split()
>> +
>> +    rootfs_orig = d.getVar('IMAGE_ROOTFS')
>> +    # We need a new rootfs directory we can delete files from. Copy to
>> +    # workdir.
>> +    new_rootfs = os.path.realpath(os.path.join(d.getVar("WORKDIR"),
>> "rootfs.%s" % taskname))
>> +
>> +    if os.path.lexists(new_rootfs):
>> +        shutil.rmtree(os.path.join(new_rootfs))
> 
> Wouldn't it be sufficient to add "new_rootfs" to "do_image[cleandirs]" ?

The directory depends on the task name, so this would be tricky I
think. You'd have to know the name upfront, which we don't, since image
types can be created by downstream layers.

>> ...>> +python cleanup_excluded_directories() {
>> +    exclude_var = d.getVar('IMAGE_ROOTFS_EXCLUDE_PATH')
>> +    if not exclude_var:
>> +        return
>> +
>> +    taskname = d.getVar("BB_CURRENTTASK")
>> +
>> +    if d.getVarFlag('do_%s' % taskname, 'respect_exclude_path') == '0':
>> +        return
>> +
>> +    import shutil
>> +
>> +    rootfs_dirs_excluded = d.getVar('IMAGE_ROOTFS')
>> +    rootfs_orig = d.getVar('IMAGE_ROOTFS_ORIG')
>> +    # This should never happen, since we should have set it to a
>> different
>> +    # directory in the prepare function.
>> +    assert rootfs_dirs_excluded != rootfs_orig
>> +
>> +    shutil.rmtree(rootfs_dirs_excluded)
>> +    d.setVar('IMAGE_ROOTFS', rootfs_orig)
>> +}
> 
> Are we sure the cleanup is needed? I can image cases where I would like
> to have a look in "rootfs_dirs_excluded" to see what how it differs from
> "rootfs_orig".

I suppose we could. Not sure what weighs heavier, cleaning up or being
able to debug. I think personally I vote for cleaning up, since it would
be easy to comment this out in a debugging situation. But I can change
it if you insist.

-- 
Kristian