[OE-core] [PATCH v1] wic: Add --exclude-path option to rootfs source plugin.

[Kristian Amlie]

On 25/11/16 17:31, Ed Bartosh wrote:
> On Fri, Nov 25, 2016 at 01:07:34PM +0100, Kristian Amlie wrote:
>> On 25/11/16 11:33, Patrick Ohly wrote:
>>> On Fri, 2016-11-25 at 11:15 +0100, Kristian Amlie wrote:
>>>> +            if os.stat(real_rootfs_dir).st_dev ==
>>>> os.stat(cr_workdir).st_dev:
>>>> +                # Optimization if both directories are on the same
>>>> file system:
>>>> +                # copy using hardlinks.
>>>> +                cp_args = "-al"
>>>> +            else:
>>>> +                cp_args = "-a"
>>>> +            exec_cmd("cp %s %s %s" % (cp_args, real_rootfs_dir,
>>>> new_rootfs))
>>>
>>> Not a full review (I'll leave that to Ed), just one thing which caught
>>> my eye: when the rootfs contains xattrs, they get lost here.
>>>
>>> Use oe.path.copyhardlinktree() instead, it also does the hardlinking
>>> trick.
>>
>> Thanks, that's a good tip! I'll include that in the next patchset.
>>
> 
> Thank you for so fast implementation!
> 
> Sorry for not answering on original e-mail. I've lost it somehow.
> 
> My comments so far:
> 
> What's the reason of insisting that path must be absolute?
> May be it's just me, but I find it a bit scaring to use absolute path in .wks
> The patch is relative to the rootfs directory from my point of view.
> 
> It also looks quite strange in the code to insist on absolute path
> +                if not os.path.isabs(path):
> +                    msger.error("Must be absolute: --exclude-path=%s" %
> 
> and then immediately making it relative:
> +
> +                while os.path.isabs(path):
> +                    path = path[1:]

Not really any strong reason. I just thought it was a logical thing to
do from a user perspective: When you're making an image you're thinking
about paths in the final image, and the path after "part" is absolute,
so I thought this one should be too.

The fact that it's made relative in the code is just an implementation
detail to make join() work correctly.

I'm fine either way, so just let me know which you prefer.

> I know, this is just a matter of taste, but I'd not use brackets around
> head, tail here. They're redundant and the code looks better without
> them from my point of view.
> +                    (head, tail) = os.path.split(remaining)

Noted, I'll remove the brackets.

> This causes rmtree to throw NotADirectoryError on files:
> +                    for entry in os.listdir(full_path):
> +                        shutil.rmtree(os.path.join(full_path, entry))

Oh, nice catch. I will fix that one as well!

Thanks for the review!

-- 
Kristian