[PATCH] bb/fetch2: fixes copying of file://dir; subdir=foo, bug 6128 and bug 6129

[PATCH] bb/fetch2: fixes copying of file://dir; subdir=foo, bug 6128 and bug 6129

[Alexander Shashkevich]

When in SRC_URI appears file://dir;subdir=foo unpacker copies 'dir' to ${WORKDIR}, not
${WORKDIR}/foo as it should be.

These changes are fixing following bugs as well:
Bug 6128 - Incorrect wildcard unpack behaviour in fetcher
Bug 6129 - Local directories unpack to a different location than local files

Signed-off-by: Alexander Shashkevich <alex@stunpix.com>
---
 lib/bb/fetch2/__init__.py | 61 +++++++++++++++++++----------------------------
 lib/bb/tests/fetch.py     | 12 ++++------
 2 files changed, 28 insertions(+), 45 deletions(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index c3dcfd2..1d9a66f 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -1343,6 +1343,11 @@ class FetchMethod(object):
         iterate = False
         file = urldata.localpath
 
+        # Localpath can't deal with 'dir/*' entries, so it converts them to '.',
+        # but it must be corrected back for local files copying
+        if urldata.basename == '*' and file.endswith('/.'):
+            file = '%s/%s' % (file.rstrip('/.'), urldata.path)
+
         try:
             unpack = bb.utils.to_boolean(urldata.parm.get('unpack'), True)
         except ValueError as exc:
@@ -1400,50 +1405,32 @@ class FetchMethod(object):
             elif file.endswith('.7z'):
                 cmd = '7za x -y %s 1>/dev/null' % file
 
+        # If 'subdir' param exists, create a dir and use it as destination for unpack cmd
+        if 'subdir' in urldata.parm:
+            unpackdir = '%s/%s' % (rootdir, urldata.parm.get('subdir'))
+            bb.utils.mkdirhier(unpackdir)
+        else:
+            unpackdir = rootdir
+
         if not unpack or not cmd:
             # If file == dest, then avoid any copies, as we already put the file into dest!
-            dest = os.path.join(rootdir, os.path.basename(file))
-            if (file != dest) and not (os.path.exists(dest) and os.path.samefile(file, dest)):
-                if os.path.isdir(file):
-                    # If for example we're asked to copy file://foo/bar, we need to unpack the result into foo/bar
-                    basepath = getattr(urldata, "basepath", None)
-                    destdir = "."
-                    if basepath and basepath.endswith("/"):
-                        basepath = basepath.rstrip("/")
-                    elif basepath:
-                        basepath = os.path.dirname(basepath)
-                    if basepath and basepath.find("/") != -1:
-                        destdir = basepath[:basepath.rfind('/')]
-                        destdir = destdir.strip('/')
-                    if destdir != "." and not os.access("%s/%s" % (rootdir, destdir), os.F_OK):
-                        os.makedirs("%s/%s" % (rootdir, destdir))
-                    cmd = 'cp -fpPR %s %s/%s/' % (file, rootdir, destdir)
-                    #cmd = 'tar -cf - -C "%d" -ps . | tar -xf - -C "%s/%s/"' % (file, rootdir, destdir)
-                else:
-                    # The "destdir" handling was specifically done for FILESPATH
-                    # items.  So, only do so for file:// entries.
-                    if urldata.type == "file" and urldata.path.find("/") != -1:
-                       destdir = urldata.path.rsplit("/", 1)[0]
-                       if urldata.parm.get('subdir') != None:
-                          destdir = urldata.parm.get('subdir') + "/" + destdir
-                    else:
-                       if urldata.parm.get('subdir') != None:
-                          destdir = urldata.parm.get('subdir')
-                       else:
-                          destdir = "."
-                    bb.utils.mkdirhier("%s/%s" % (rootdir, destdir))
-                    cmd = 'cp -f %s %s/%s/' % (file, rootdir, destdir)
+            dest = os.path.join(unpackdir, os.path.basename(file))
+            if file != dest and not (os.path.exists(dest) and os.path.samefile(file, dest)):
+                destdir = '.'
+                # For file:// entries all intermediate dirs in path must be created at destination
+                if urldata.type == "file":
+                    urlpath = urldata.path.rstrip('/') # Trailing '/' does a copying to wrong place
+                    if urlpath.find("/") != -1:
+                        destdir = urlpath.rsplit("/", 1)[0] + '/'
+                        bb.utils.mkdirhier("%s/%s" % (unpackdir, destdir))
+                cmd = 'cp -fpPR %s %s' % (file, destdir)
 
         if not cmd:
             return
 
-        # Change to subdir before executing command
+        # Change to unpackdir before executing command
         save_cwd = os.getcwd();
-        os.chdir(rootdir)
-        if 'subdir' in urldata.parm:
-            newdir = ("%s/%s" % (rootdir, urldata.parm.get('subdir')))
-            bb.utils.mkdirhier(newdir)
-            os.chdir(newdir)
+        os.chdir(unpackdir)
 
         path = data.getVar('PATH', True)
         if path:
diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index 81b22d0..850e2ba 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -451,9 +451,7 @@ class FetcherLocalTest(FetcherTest):
 
     def test_local_wildcard(self):
         tree = self.fetchUnpack(['file://a', 'file://dir/*'])
-        # FIXME: this is broken - it should return ['a', 'dir/c', 'dir/d', 'dir/subdir/e']
-        # see https://bugzilla.yoctoproject.org/show_bug.cgi?id=6128
-        self.assertEqual(tree, ['a', 'b', 'dir/c', 'dir/d', 'dir/subdir/e'])
+        self.assertEqual(tree, ['a',  'dir/c', 'dir/d', 'dir/subdir/e'])
 
     def test_local_dir(self):
         tree = self.fetchUnpack(['file://a', 'file://dir'])
@@ -461,17 +459,15 @@ class FetcherLocalTest(FetcherTest):
 
     def test_local_subdir(self):
         tree = self.fetchUnpack(['file://dir/subdir'])
-        # FIXME: this is broken - it should return ['dir/subdir/e']
-        # see https://bugzilla.yoctoproject.org/show_bug.cgi?id=6129
-        self.assertEqual(tree, ['subdir/e'])
+        self.assertEqual(tree, ['dir/subdir/e'])
 
     def test_local_subdir_file(self):
         tree = self.fetchUnpack(['file://dir/subdir/e'])
         self.assertEqual(tree, ['dir/subdir/e'])
 
     def test_local_subdirparam(self):
-        tree = self.fetchUnpack(['file://a;subdir=bar'])
-        self.assertEqual(tree, ['bar/a'])
+        tree = self.fetchUnpack(['file://a;subdir=bar', 'file://dir;subdir=foo/moo'])
+        self.assertEqual(tree, ['bar/a', 'foo/moo/dir/c', 'foo/moo/dir/d', 'foo/moo/dir/subdir/e'])
 
     def test_local_deepsubdirparam(self):
         tree = self.fetchUnpack(['file://dir/subdir/e;subdir=bar'])
-- 
2.1.4

Re: [PATCH] bb/fetch2: fixes copying of file://dir; subdir=foo, bug 6128 and bug 6129

[Richard Purdie]

On Thu, 2016-02-25 at 18:32 +0200, Alexander Shashkevich wrote:
> When in SRC_URI appears file://dir;subdir=foo unpacker copies 'dir'
> to ${WORKDIR}, not
> ${WORKDIR}/foo as it should be.
> 
> These changes are fixing following bugs as well:
> Bug 6128 - Incorrect wildcard unpack behaviour in fetcher
> Bug 6129 - Local directories unpack to a different location than
> local files

Sadly, this change isn't without side effects. For example:

https://autobuilder.yoctoproject.org/main/builders/nightly-x86-64/builds/680/steps/Building%20Toolchain%20Images/logs/stdio

| NOTE: Unpacking /home/pokybuild/yocto-autobuilder/yocto-worker/nightly-x86-64/build/scripts/runqemu to /home/pokybuild/yocto-autobuilder/yocto-worker/nightly-x86-64/build/build/tmp/work/i686-nativesdk-pokysdk-linux/nativesdk-qemu-helper/1.0-r9/
| cp: `/home/pokybuild/yocto-autobuilder/yocto-worker/nightly-x86-64/build/scripts/runqemu' and `/home/pokybuild/yocto-autobuilder/yocto-worker/nightly-x86-64/build/scripts/runqemu' are the same file
| DEBUG: Python function base_do_unpack finished
| DEBUG: Python function do_unpack finished

Cheers,

Richard

Re: [PATCH] bb/fetch2: fixes copying of file://dir; subdir=foo, bug 6128 and bug 6129

[Sasha Shashkevich]

> On Feb 26, 2016, at 12:51, Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
> 
> On Thu, 2016-02-25 at 18:32 +0200, Alexander Shashkevich wrote:
>> When in SRC_URI appears file://dir;subdir=foo unpacker copies 'dir'
>> to ${WORKDIR}, not
>> ${WORKDIR}/foo as it should be.
>> 
>> These changes are fixing following bugs as well:
>> Bug 6128 - Incorrect wildcard unpack behaviour in fetcher
>> Bug 6129 - Local directories unpack to a different location than
>> local files
> 
> Sadly, this change isn't without side effects. For example:
> 
> https://autobuilder.yoctoproject.org/main/builders/nightly-x86-64/builds/680/steps/Building%20Toolchain%20Images/logs/stdio
> 
> | NOTE: Unpacking /home/pokybuild/yocto-autobuilder/yocto-worker/nightly-x86-64/build/scripts/runqemu to /home/pokybuild/yocto-autobuilder/yocto-worker/nightly-x86-64/build/build/tmp/work/i686-nativesdk-pokysdk-linux/nativesdk-qemu-helper/1.0-r9/
> | cp: `/home/pokybuild/yocto-autobuilder/yocto-worker/nightly-x86-64/build/scripts/runqemu' and `/home/pokybuild/yocto-autobuilder/yocto-worker/nightly-x86-64/build/scripts/runqemu' are the same file
> | DEBUG: Python function base_do_unpack finished
> | DEBUG: Python function do_unpack finished


Despite all tests were successfully passed, something remains incorrect. Very sensitive feature. I'll check what is wrong and supply additional test cases with updated patch. Seems I need to build several configurations locally before submitting such patches – just tests are not enough.

Regards,
Alexander

Re: [PATCH] bb/fetch2: fixes copying of file://dir; subdir=foo, bug 6128 and bug 6129

[Richard Purdie]

On Fri, 2016-02-26 at 13:30 +0200, Sasha Shashkevich wrote:
> > On Feb 26, 2016, at 12:51, Richard Purdie <
> > richard.purdie@linuxfoundation.org> wrote:
> > 
> > On Thu, 2016-02-25 at 18:32 +0200, Alexander Shashkevich wrote:
> > > When in SRC_URI appears file://dir;subdir=foo unpacker copies
> > > 'dir'
> > > to ${WORKDIR}, not
> > > ${WORKDIR}/foo as it should be.
> > > 
> > > These changes are fixing following bugs as well:
> > > Bug 6128 - Incorrect wildcard unpack behaviour in fetcher
> > > Bug 6129 - Local directories unpack to a different location than
> > > local files
> > 
> > Sadly, this change isn't without side effects. For example:
> > 
> > https://autobuilder.yoctoproject.org/main/builders/nightly-x86-64/b
> > uilds/680/steps/Building%20Toolchain%20Images/logs/stdio
> > 
> > > NOTE: Unpacking /home/pokybuild/yocto-autobuilder/yocto
> > > -worker/nightly-x86-64/build/scripts/runqemu to
> > > /home/pokybuild/yocto-autobuilder/yocto-worker/nightly-x86
> > > -64/build/build/tmp/work/i686-nativesdk-pokysdk-linux/nativesdk
> > > -qemu-helper/1.0-r9/
> > > cp: `/home/pokybuild/yocto-autobuilder/yocto-worker/nightly-x86
> > > -64/build/scripts/runqemu' and `/home/pokybuild/yocto
> > > -autobuilder/yocto-worker/nightly-x86-64/build/scripts/runqemu'
> > > are the same file
> > > DEBUG: Python function base_do_unpack finished
> > > DEBUG: Python function do_unpack finished
> 
> 
> Despite all tests were successfully passed, something remains
> incorrect. Very sensitive feature. I'll check what is wrong and
> supply additional test cases with updated patch. Seems I need to
> build several configurations locally before submitting such patches –
> just tests are not enough.

Yes, sadly our tests aren't entirely complete. I'd much appreciate
added test cases if we can figure out what the issue is.

This change does introduce a change in behaviour, admittedly one where
we think the current behaviour is incorrect. It is possible recipes are
relying on that incorrect behaviour though and they may need adjusting.

Cheers,

Richard

Re: [PATCH] bb/fetch2: fixes copying of file://dir; subdir=foo, bug 6128 and bug 6129

[Richard Purdie]

On Fri, 2016-02-26 at 11:46 +0000, Richard Purdie wrote:
> On Fri, 2016-02-26 at 13:30 +0200, Sasha Shashkevich wrote:
> > > On Feb 26, 2016, at 12:51, Richard Purdie <
> > > richard.purdie@linuxfoundation.org> wrote:
> > > 
> > > On Thu, 2016-02-25 at 18:32 +0200, Alexander Shashkevich wrote:
> > > > When in SRC_URI appears file://dir;subdir=foo unpacker copies
> > > > 'dir'
> > > > to ${WORKDIR}, not
> > > > ${WORKDIR}/foo as it should be.
> > > > 
> > > > These changes are fixing following bugs as well:
> > > > Bug 6128 - Incorrect wildcard unpack behaviour in fetcher
> > > > Bug 6129 - Local directories unpack to a different location
> > > > than
> > > > local files
> > > 
> > > Sadly, this change isn't without side effects. For example:
> > > 
> > > https://autobuilder.yoctoproject.org/main/builders/nightly-x86-64
> > > /b
> > > uilds/680/steps/Building%20Toolchain%20Images/logs/stdio
> > > 
> > > > NOTE: Unpacking /home/pokybuild/yocto-autobuilder/yocto
> > > > -worker/nightly-x86-64/build/scripts/runqemu to
> > > > /home/pokybuild/yocto-autobuilder/yocto-worker/nightly-x86
> > > > -64/build/build/tmp/work/i686-nativesdk-pokysdk-linux/nativesdk
> > > > -qemu-helper/1.0-r9/
> > > > cp: `/home/pokybuild/yocto-autobuilder/yocto-worker/nightly-x86
> > > > -64/build/scripts/runqemu' and `/home/pokybuild/yocto
> > > > -autobuilder/yocto-worker/nightly-x86-64/build/scripts/runqemu'
> > > > are the same file
> > > > DEBUG: Python function base_do_unpack finished
> > > > DEBUG: Python function do_unpack finished
> > 
> > 
> > Despite all tests were successfully passed, something remains
> > incorrect. Very sensitive feature. I'll check what is wrong and
> > supply additional test cases with updated patch. Seems I need to
> > build several configurations locally before submitting such patches
> > –
> > just tests are not enough.
> 
> Yes, sadly our tests aren't entirely complete. I'd much appreciate
> added test cases if we can figure out what the issue is.
> 
> This change does introduce a change in behaviour, admittedly one
> where
> we think the current behaviour is incorrect. It is possible recipes
> are
> relying on that incorrect behaviour though and they may need
> adjusting.

FWIW, a:

-                if urldata.type == "file":
+                if urldata.type == "file" and not urldata.path.startswith("/"):

did seem to fix the failing recipe which was doing something valid. What 
effect elsewhere this has I haven't checked.

Cheers,

Richard

Re: [PATCH] bb/fetch2: fixes copying of file://dir; subdir=foo, bug 6128 and bug 6129

[Richard Purdie]

On Fri, 2016-02-26 at 16:51 +0000, Richard Purdie wrote:
> On Fri, 2016-02-26 at 11:46 +0000, Richard Purdie wrote:
> > On Fri, 2016-02-26 at 13:30 +0200, Sasha Shashkevich wrote:
> > > > On Feb 26, 2016, at 12:51, Richard Purdie <
> > > > richard.purdie@linuxfoundation.org> wrote:
> > > > 
> > > > On Thu, 2016-02-25 at 18:32 +0200, Alexander Shashkevich wrote:
> > > > > When in SRC_URI appears file://dir;subdir=foo unpacker copies
> > > > > 'dir'
> > > > > to ${WORKDIR}, not
> > > > > ${WORKDIR}/foo as it should be.
> > > > > 
> > > > > These changes are fixing following bugs as well:
> > > > > Bug 6128 - Incorrect wildcard unpack behaviour in fetcher
> > > > > Bug 6129 - Local directories unpack to a different location
> > > > > than
> > > > > local files
> > > > 
> > > > Sadly, this change isn't without side effects. For example:
> > > > 
> > > > https://autobuilder.yoctoproject.org/main/builders/nightly-x86-
> > > > 64
> > > > /b
> > > > uilds/680/steps/Building%20Toolchain%20Images/logs/stdio
> > > > 
> > > > > NOTE: Unpacking /home/pokybuild/yocto-autobuilder/yocto
> > > > > -worker/nightly-x86-64/build/scripts/runqemu to
> > > > > /home/pokybuild/yocto-autobuilder/yocto-worker/nightly-x86
> > > > > -64/build/build/tmp/work/i686-nativesdk-pokysdk
> > > > > -linux/nativesdk
> > > > > -qemu-helper/1.0-r9/
> > > > > cp: `/home/pokybuild/yocto-autobuilder/yocto-worker/nightly
> > > > > -x86
> > > > > -64/build/scripts/runqemu' and `/home/pokybuild/yocto
> > > > > -autobuilder/yocto-worker/nightly-x86
> > > > > -64/build/scripts/runqemu'
> > > > > are the same file
> > > > > DEBUG: Python function base_do_unpack finished
> > > > > DEBUG: Python function do_unpack finished
> > > 
> > > 
> > > Despite all tests were successfully passed, something remains
> > > incorrect. Very sensitive feature. I'll check what is wrong and
> > > supply additional test cases with updated patch. Seems I need to
> > > build several configurations locally before submitting such
> > > patches
> > > –
> > > just tests are not enough.
> > 
> > Yes, sadly our tests aren't entirely complete. I'd much appreciate
> > added test cases if we can figure out what the issue is.
> > 
> > This change does introduce a change in behaviour, admittedly one
> > where
> > we think the current behaviour is incorrect. It is possible recipes
> > are
> > relying on that incorrect behaviour though and they may need
> > adjusting.
> 
> FWIW, a:
> 
> -                if urldata.type == "file":
> +                if urldata.type == "file" and not
> urldata.path.startswith("/"):
> 
> did seem to fix the failing recipe which was doing something valid.
> What 
> effect elsewhere this has I haven't checked.

I'm wrong, the correct fix appears to be:

-                    urlpath = urldata.path.rstrip('/') # Trailing '/' does a copying to wrong place
+                    # Trailing '/' does a copying to wrong place
+                    urlpath = urldata.path.rstrip('/')
+                    # Want files places relative to cwd so no leading '/'
+                    urldata.path.lstrip('/')

and we're missing a test case with absolute file:// urls.

Cheers,

Richard

Re: [PATCH] bb/fetch2: fixes copying of file://dir; subdir=foo, bug 6128 and bug 6129

[Sasha Shashkevich]

[-- Attachment #1: Type: text/plain, Size: 1775 bytes --]


>>>>> NOTE: Unpacking /home/pokybuild/yocto-autobuilder/yocto
>>>>> -worker/nightly-x86-64/build/scripts/runqemu to
>>>>> /home/pokybuild/yocto-autobuilder/yocto-worker/nightly-x86
>>>>> -64/build/build/tmp/work/i686-nativesdk-pokysdk-linux/nativesdk
>>>>> -qemu-helper/1.0-r9/
>>>>> cp: `/home/pokybuild/yocto-autobuilder/yocto-worker/nightly-x86
>>>>> -64/build/scripts/runqemu' and `/home/pokybuild/yocto
>>>>> -autobuilder/yocto-worker/nightly-x86-64/build/scripts/runqemu'
>>>>> are the same file
>>>>> DEBUG: Python function base_do_unpack finished
>>>>> DEBUG: Python function do_unpack finished
>>> 
>>> 
>>> Despite all tests were successfully passed, something remains
>>> incorrect. Very sensitive feature. I'll check what is wrong and
>>> supply additional test cases with updated patch. Seems I need to
>>> build several configurations locally before submitting such patches
>>> –
>>> just tests are not enough.
>> 
>> Yes, sadly our tests aren't entirely complete. I'd much appreciate
>> added test cases if we can figure out what the issue is.
>> 
>> This change does introduce a change in behaviour, admittedly one
>> where
>> we think the current behaviour is incorrect. It is possible recipes
>> are
>> relying on that incorrect behaviour though and they may need
>> adjusting.
> 
> FWIW, a:
> 
> -                if urldata.type == "file":
> +                if urldata.type == "file" and not urldata.path.startswith("/"):
> 
> did seem to fix the failing recipe which was doing something valid. What 
> effect elsewhere this has I haven't checked.
> 

Yep, that's the reason: SRC_URI could contain absolute paths. Thank you for finding that. I'll check the rest of build and send patch #3.

Regards,
Alexander

[-- Attachment #2: Type: text/html, Size: 7551 bytes --]

Re: [PATCH] bb/fetch2: fixes copying of file://dir; subdir=foo, bug 6128 and bug 6129

[Sasha Shashkevich]

[-- Attachment #1: Type: text/plain, Size: 1335 bytes --]


>>>> Despite all tests were successfully passed, something remains
>>>> incorrect. Very sensitive feature. I'll check what is wrong and
>>>> supply additional test cases with updated patch. Seems I need to
>>>> build several configurations locally before submitting such patches
>>>> –
>>>> just tests are not enough.
>>> 
>>> Yes, sadly our tests aren't entirely complete. I'd much appreciate
>>> added test cases if we can figure out what the issue is.
>>> 
>>> This change does introduce a change in behaviour, admittedly one
>>> where
>>> we think the current behaviour is incorrect. It is possible recipes
>>> are
>>> relying on that incorrect behaviour though and they may need
>>> adjusting.
>> 
>> FWIW, a:
>> 
>> -                if urldata.type == "file":
>> +                if urldata.type == "file" and not urldata.path.startswith("/"):
>> 
>> did seem to fix the failing recipe which was doing something valid. What 
>> effect elsewhere this has I haven't checked.
>> 
> 
> Yep, that's the reason: SRC_URI could contain absolute paths. Thank you for finding that. I'll check the rest of build and send patch #3.

Some other recipes are still failing even with your suggestion. WIP. This isn't my high prio task in project, so it takes some time to fix all cases.

Regards,
Alexander


[-- Attachment #2: Type: text/html, Size: 7154 bytes --]