Bug 5968

Summary: oe/patch.py: PATCHTOOL = "patch" doesn't work
Product: [Build System, Metadata & Runtime] OE-Core Reporter: Robert Yang <liezhi.yang>
Component: coreAssignee: Robert Yang <liezhi.yang>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: Low CC: andrei, jessica.zhang, kergoth, Martin.Jansa, meta.mr.watcher, meta.watcher, ross.burton, sgw
Version: 1.6   
Target Milestone: 1.8   
Hardware: x86   
OS: Multiple   
Whiteboard:
OS type for building Yocto: --- Type of Regression: ---
Verified: Documentation change: Don't know

Description Robert Yang 2014-03-14 08:57:00 UTC
On openSUSE 12.2 x86_64: (not very sure about other hosts)

set PATCHTOOL = "patch" in conf/local.conf, and:

$ bitbake perl-native

then we will get this error:

ERROR: Command Error: exit status: 1  Output:
File Configure is read-only; refusing to patch
1 out of 1 hunk ignored -- saving rejects to file Configure.rej
ERROR: Function failed: patch_do_patch
ERROR: Logfile of failure stored in: /buildarea/lyang1/testpp/tmp/work/x86_64-linux/perl-native/5.14.3-r0/temp/log.do_patch.23971
ERROR: Task 1 (/buildarea/lyang1/poky/meta/recipes-devtools/perl/perl-native_5.14.3.bb, do_patch) failed with exit code '1'

Check the Configure:
$ ls -l tmp/work/x86_64-linux/perl-native/5.14.3-r0/perl-5.14.3/Configure
-r-xr-xr-x 1 lyang1 users 539754 Oct 11  2012 tmp/work/x86_64-linux/perl-native/5.14.3-r0/perl-5.14.3/Configure

We don't have the "w" permission indeeded, maybe OpenSuse's patch acts differently.
Comment 1 Robert Yang 2014-03-14 09:13:39 UTC
And we should set TMPDIR and unset it in meta/lib/oe/patch.py rather than in patch.bbclass, here is a draft patch:

diff --git a/meta/classes/patch.bbclass b/meta/classes/patch.bbclass
index 86c65b3..2ade0a8 100644
--- a/meta/classes/patch.bbclass
+++ b/meta/classes/patch.bbclass
@@ -139,13 +139,6 @@ python patch_do_patch() {
     path = os.getenv('PATH')
     os.putenv('PATH', d.getVar('PATH', True))
 
-    # We must use one TMPDIR per process so that the "patch" processes
-    # don't generate the same temp file name.
-
-    import tempfile
-    process_tmpdir = tempfile.mkdtemp()
-    os.environ['TMPDIR'] = process_tmpdir
-
     for patch in src_patches(d):
         _, _, local, _, _, parm = bb.fetch.decodeurl(patch)
 
@@ -168,15 +161,11 @@ python patch_do_patch() {
         try:
             patchset.Import({"file":local, "strippath": parm['striplevel']}, True)
         except Exception as exc:
-            bb.utils.remove(process_tmpdir, True)
             bb.fatal(str(exc))
         try:
             resolver.Resolve()
         except bb.BBHandledException as e:
-            bb.utils.remove(process_tmpdir, True)
             bb.fatal(str(e))
-
-    bb.utils.remove(process_tmpdir, True)
 }
 patch_do_patch[vardepsexclude] = "PATCHRESOLVE"
 
diff --git a/meta/lib/oe/patch.py b/meta/lib/oe/patch.py
index b085c9d..71c0589 100644
--- a/meta/lib/oe/patch.py
+++ b/meta/lib/oe/patch.py
@@ -138,6 +138,8 @@ class PatchTree(PatchSet):
         self.patches.insert(i, patch)
 
     def _applypatch(self, patch, force = False, reverse = False, run = True):
+        import tempfile
+
         shellcmd = ["cat", patch['file'], "|", "patch", "-p", patch['strippath']]
         if reverse:
             shellcmd.append('-R')
@@ -154,7 +156,16 @@ class PatchTree(PatchSet):
             return
 
         shellcmd.pop(len(shellcmd) - 1)
-        output = runcmd(["sh", "-c", " ".join(shellcmd)], self.dir)
+
+        # We must use one TMPDIR per process so that the "patch" processes
+        # don't generate the same temp file name.
+        process_tmpdir = tempfile.mkdtemp()
+        os.environ['TMPDIR'] = process_tmpdir
+        try:
+            output = runcmd(["sh", "-c", " ".join(shellcmd)], self.dir)
+        finally:
+            bb.utils.remove(process_tmpdir, True)
+            os.unsetenv('TMPDIR')
 
         if not reverse:
             self._appendPatchFile(patch['file'], patch['strippath'])
Comment 2 Ross Burton 2014-03-14 09:52:52 UTC
This has the same problem: if TMPDIR was already set in the environment you wipe it.

I was thinking more along the lines of passing TMPDIR=/my/new/tmp/path directly to sh in the runcmd call so it's only valid in the shell.
Comment 3 Ross Burton 2014-03-14 09:53:22 UTC
Then again is there a reason to use patch instead of quilt here?
Comment 4 Robert Yang 2014-03-14 10:11:11 UTC
(In reply to comment #2)
> This has the same problem: if TMPDIR was already set in the environment you
> wipe it.
> 
> I was thinking more along the lines of passing TMPDIR=/my/new/tmp/path
> directly to sh in the runcmd call so it's only valid in the shell.

Sounds reasonable.

And there is no reason to use PATCHTOOL = "patch", I just think that it is a choice, and it should work, should it ?
Comment 5 Ross Burton 2014-03-14 10:12:33 UTC
Well, if it's just a dead codepath that others no advantage over quilt, why should we even keep it?
Comment 6 Robert Yang 2014-03-14 10:15:41 UTC
(In reply to comment #5)
> Well, if it's just a dead codepath that others no advantage over quilt, why
> should we even keep it?

Agreed, let's try to drop a patch to oe-core mailing list.
Comment 7 Martin Jansa 2014-03-14 12:09:45 UTC
There is at least one still useful case where PATCHTOOL is set:

commit a29c71024c304263bfb0732c870f20c1a6af7f64
Author: Martin Jansa <martin.jansa@gmail.com>
Date:   Wed Dec 4 18:33:18 2013 +0100

    icecc-create-env-native: Drop FILESPATH, document PATCHTOOL
    
    * moving icecc-create-env to BPN allows to drop FILESPATH
    * document PATCHTOOL, because it's not easy to guess why it's needed

With old archiver bbclasses I was using it too for most recipes:

+# quilt leaves patches directories with symlinks to .patch files, revealing build paths in source archives
+PATCHTOOL = "patch"
+# fail to patch with "patch":
+PATCHTOOL_pn-busybox = "quilt"
+PATCHTOOL_pn-ruby = "quilt"
+PATCHTOOL_pn-ruby-native = "quilt"
Comment 8 Andrei Gherzan 2014-08-13 14:48:43 UTC
Yes. There is a reason to use something else instead of quilt. Quilt does not support binary diffs. Git does, so I would need git to be used as patches tool.
Comment 9 Martin Jansa 2014-08-13 15:00:19 UTC
We're also using PATCHTOOL = "patch" in builds with archiver.bbclass, because quilt leaves patches directory which reveals build paths (I know we should rather extend archiver.bbclass to exclude them, but switching PATCHTOOL was simplest work around at that time).
Comment 10 Ross Burton 2015-02-11 18:06:26 UTC
Pretty sure this was fixed with oe-core 51ea4378864f1468df2ca282a84f78a17d6861aa.
Comment 11 Andrei Gherzan 2015-04-30 10:50:30 UTC
Works on fido. Thanks.