[PATCH] bb/fetch2: broken functionality for entries like file://dir; subdir=foo in SRC_URI

[PATCH] bb/fetch2: broken functionality for entries like file://dir; subdir=foo in SRC_URI

[Alexander Shashkevich]

For SRC_URI entries like file://dir1;subdir=foo1 and file://dir2/dir3;subdir=foo2/foo3
subdirectories ${WORKDIR}/foo1 and ${WORKDIR}/foo2/foo3 are created, but dir1 and dir2/dir3 are
only copied to ${WORKDIR}/dir1 and ${WORKDIR}/dir2/dir. Correct way: directories must be copied/unpacked
into '${WORKDIR}/foo1/dir1' and '${WORKDIR}/foo2/foo3/dir2/dir' respectively.

Signed-off-by: Alexander Shashkevich <alex@stunpix.com>
---
 lib/bb/fetch2/__init__.py | 55 ++++++++++++++++-------------------------------
 1 file changed, 18 insertions(+), 37 deletions(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index c3dcfd2..87486b2 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -1400,50 +1400,31 @@ class FetchMethod(object):
             elif file.endswith('.7z'):
                 cmd = '7za x -y %s 1>/dev/null' % file
 
+        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 '/' makes copy 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 workdir 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:
-- 
2.1.4

Re: [PATCH] bb/fetch2: broken functionality for entries like file://dir; subdir=foo in SRC_URI

[Richard Purdie]

On Mon, 2016-02-22 at 20:34 +0200, Alexander Shashkevich wrote:
> For SRC_URI entries like file://dir1;subdir=foo1 and
> file://dir2/dir3;subdir=foo2/foo3
> subdirectories ${WORKDIR}/foo1 and ${WORKDIR}/foo2/foo3 are created,
> but dir1 and dir2/dir3 are
> only copied to ${WORKDIR}/dir1 and ${WORKDIR}/dir2/dir. Correct way:
> directories must be copied/unpacked
> into '${WORKDIR}/foo1/dir1' and '${WORKDIR}/foo2/foo3/dir2/dir'
> respectively.

Where we have cases like this, I'd ask that you add a test case to
lib/bb/tests/ which illustrates the problem and ensures we don't
regress in future. We've had far too many problems with corner cases
and the fetcher :(

I need to properly review the patch but wanted to mention this now.

Cheers,

Richard

Re: [PATCH] bb/fetch2: broken functionality for entries like file://dir; subdir=foo in SRC_URI

[Sasha Shashkevich]

> On Feb 22, 2016, at 22:51, Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
> 
> On Mon, 2016-02-22 at 20:34 +0200, Alexander Shashkevich wrote:
>> For SRC_URI entries like file://dir1;subdir=foo1 and
>> file://dir2/dir3;subdir=foo2/foo3
>> subdirectories ${WORKDIR}/foo1 and ${WORKDIR}/foo2/foo3 are created,
>> but dir1 and dir2/dir3 are
>> only copied to ${WORKDIR}/dir1 and ${WORKDIR}/dir2/dir. Correct way:
>> directories must be copied/unpacked
>> into '${WORKDIR}/foo1/dir1' and '${WORKDIR}/foo2/foo3/dir2/dir'
>> respectively.
> 
> Where we have cases like this, I'd ask that you add a test case to
> lib/bb/tests/ which illustrates the problem and ensures we don't
> regress in future. We've had far too many problems with corner cases
> and the fetcher :(
> 
> I need to properly review the patch but wanted to mention this now.

Good point. Just launched test cases for my changes and one regression appeared. Bad. I'll fix regression and add test cases.

Thank you. :)

Alexander