Hi!
I've tried -Su again... I've just run into a very "hard to fix" error.
[ngaba@Arch ~]$ LANG=C sudo pacman -Su --ignore mplayer ... xulrunner: /usr/include/xulrunner-1.9 exists in filesystem xulrunner: /usr/lib/xulrunner-1.9 exists in filesystem xulrunner: /usr/lib/xulrunner-devel-1.9 exists in filesystem xulrunner: /usr/share/idl/xulrunner-1.9 exists in filesystem ...
These are directories... But in the package we have symlinks.
Bye
Nagy Gabor wrote:
Hi!
I've tried -Su again... I've just run into a very "hard to fix" error.
[ngaba@Arch ~]$ LANG=C sudo pacman -Su --ignore mplayer ... xulrunner: /usr/include/xulrunner-1.9 exists in filesystem xulrunner: /usr/lib/xulrunner-1.9 exists in filesystem xulrunner: /usr/lib/xulrunner-devel-1.9 exists in filesystem xulrunner: /usr/share/idl/xulrunner-1.9 exists in filesystem ...
These are directories... But in the package we have symlinks.
I ran into this problem 10 seconds before receiving your mail :P
Anyway, if we removed xulrunner-1.9 before installing xulrunner-1.9.0.1, this error shouldn't happen, because the files in these directories apparently all belong to xulrunner. Otherwise pacman could check for this propriety.
I ran into this problem 10 seconds before receiving your mail :P
Anyway, if we removed xulrunner-1.9 before installing xulrunner-1.9.0.1, this error shouldn't happen, because the files in these directories apparently all belong to xulrunner. Otherwise pacman could check for this propriety.
Yes. We should do ~query_fileowner() for all files in that dir (and recurse to subdirs). For efficiency we should start with oldpkg, because query_fileowner is not a fast operation (but the situation of this thread is not common, so the speed is not so important here). But this is not so trivial, we could consider NoUpgrade, ".pacsave" stuffs too (and I may miss something here), if we want to _simulate_ our transaction perfectly.
Bye
On Sat, Jul 19, 2008 at 2:25 PM, Nagy Gabor ngaba@bibl.u-szeged.hu wrote:
Hi!
I've tried -Su again... I've just run into a very "hard to fix" error.
[ngaba@Arch ~]$ LANG=C sudo pacman -Su --ignore mplayer ... xulrunner: /usr/include/xulrunner-1.9 exists in filesystem xulrunner: /usr/lib/xulrunner-1.9 exists in filesystem xulrunner: /usr/lib/xulrunner-devel-1.9 exists in filesystem xulrunner: /usr/share/idl/xulrunner-1.9 exists in filesystem ...
These are directories... But in the package we have symlinks.
I thought this was a regression caused by this fix : http://bugs.archlinux.org/task/8156
But apparently this fix made it into pacman 3.1, and 3.1 didn't have the above problem. So now I am confused...
On Sat, Jul 19, 2008 at 2:25 PM, Nagy Gabor ngaba@bibl.u-szeged.hu wrote:
Hi!
I've tried -Su again... I've just run into a very "hard to fix" error.
[ngaba@Arch ~]$ LANG=C sudo pacman -Su --ignore mplayer ... xulrunner: /usr/include/xulrunner-1.9 exists in filesystem xulrunner: /usr/lib/xulrunner-1.9 exists in filesystem xulrunner: /usr/lib/xulrunner-devel-1.9 exists in filesystem xulrunner: /usr/share/idl/xulrunner-1.9 exists in filesystem ...
These are directories... But in the package we have symlinks.
I thought this was a regression caused by this fix : http://bugs.archlinux.org/task/8156
But apparently this fix made it into pacman 3.1, and 3.1 didn't have the above problem. So now I am confused...
I think this is not a regression, but a behaviour change. What did the "old" pacman do when we have a real symlink<->dir conflict? I mean a conflict which won't disappear after the upgrade_remove part: fileconflict003 should pass.
Bye
P.S.: I will create a pactest for this case.
On Tue, Jul 22, 2008 at 12:34 PM, Nagy Gabor ngaba@bibl.u-szeged.hu wrote:
I think this is not a regression, but a behaviour change. What did the "old" pacman do when we have a real symlink<->dir conflict? I mean a conflict which won't disappear after the upgrade_remove part: fileconflict003 should pass.
Yeah sure, that is why I still called it a fix. It fixed some cases (the fileconflict003 one), but caused a regression in others (like xulrunner case). But right, we can just say behavior change. Still, I am confused, when did this behavior change? I thought it was caused by the patch in the above bug report, but this happened before 3.1. And apparently 3.1 behavior is different than 3.2, because 3.1 handled xulrunner upgrade fine, and not 3.2. Maybe some other changes that happened in 3.2 caused this behavior change in combination to that "fix for fileconflict003" patch ?
P.S.: I will create a pactest for this case.
That would be nice. And even better if you could find out when it broke :)
On Tue, Jul 22, 2008 at 12:34 PM, Nagy Gabor ngaba@bibl.u-szeged.hu wrote:
I think this is not a regression, but a behaviour change. What did the "old" pacman do when we have a real symlink<->dir conflict? I mean a conflict which won't disappear after the upgrade_remove part: fileconflict003 should pass.
Yeah sure, that is why I still called it a fix. It fixed some cases (the fileconflict003 one), but caused a regression in others (like xulrunner case). But right, we can just say behavior change. Still, I am confused, when did this behavior change? I thought it was caused by the patch in the above bug report, but this happened before 3.1. And apparently 3.1 behavior is different than 3.2, because 3.1 handled xulrunner upgrade fine, and not 3.2. Maybe some other changes that happened in 3.2 caused this behavior change in combination to that "fix for fileconflict003" patch ?
P.S.: I will create a pactest for this case.
That would be nice. And even better if you could find out when it broke :)
I will go offline until evening. Cannot git be used as a testing machine (with fileconflict004.py) here?
Bye
On Tue, Jul 22, 2008 at 1:31 PM, Nagy Gabor ngaba@bibl.u-szeged.hu wrote:
I will go offline until evening. Cannot git be used as a testing machine (with fileconflict004.py) here?
Hmm reading man git-bisect, this seems possible. We just need some scripts to return 1 when fileconflict004 fails, and 0 when it pass. It could be pactest directly after patching it, or just a wrapper on pactest which greps "PASS = 1" or something like that.
Otherwise you can always manually use fileconflict004 as a test for doing a bisection using git, but then, it is probably faster to only do test before any commits related to fileconflict, rather than a blind testing.
Anyway, I still did a git bisect with manual testing for fun, and the result is funny :)
"""" 584ffa6aef13d0933ad4930ab9cb70d3af2977ff is first bad commit commit 584ffa6aef13d0933ad4930ab9cb70d3af2977ff Author: Dan McGee dan@archlinux.org Date: Mon May 12 20:49:18 2008 -0500
Remove an outdated exception check in file conflict code
This has been around since at least pacman 2.9.8. Frugalware just dumped it in commit 113ec73bfcfdc, and deleting it here and running pactest shows that nothing that we have actually tested changes. If someone can pactest the edge case where this is needed, then show me the money.
Signed-off-by: Dan McGee dan@archlinux.org
:040000 040000 ea3e9f52732a5412f2e77e2a1060c3247636de7b a234d7e4ca725edc301f696640d897ce54dfb3b8 M lib """"
Well here you go, the pactest is called fileconflict004, courtesy of Nagy ;)
Signed-off-by: Dan McGee <dan@archlinux.org>:040000 040000 ea3e9f52732a5412f2e77e2a1060c3247636de7b a234d7e4ca725edc301f696640d897ce54dfb3b8 M lib """"
OMG. That part is totally unrelated here, but it makes our fileconflict003.py pass (due to its "bug");-). Obviously that won't solve our problem (unfortunately).
Bye
OMG. That part is totally unrelated here, but it makes our fileconflict003.py pass (due to its "bug");-). Obviously that won't solve our problem (unfortunately).
typo: fileconflict004.py
For example, this will fail: ---------------------------- self.description = "foo"
p1 = pmpkg("pkg1", "1.0-1") p1.files = ["test/", "test/file1"] self.addpkg2db("local", p1)
p2 = pmpkg("pkg2") p2.files = ["test/file2"] self.addpkg2db("local", p2)
p3 = pmpkg("pkg1", "2.0-1") p3.files = ["test2/", "test2/file3", "test -> test2"] self.addpkg2db("sync", p3)
self.args = "-S pkg1"
self.addrule("PACMAN_RETCODE=1")
On Tue, Jul 22, 2008 at 8:20 AM, Xavier shiningxc@gmail.com wrote:
On Tue, Jul 22, 2008 at 1:31 PM, Nagy Gabor ngaba@bibl.u-szeged.hu wrote:
I will go offline until evening. Cannot git be used as a testing machine (with fileconflict004.py) here?
Hmm reading man git-bisect, this seems possible. We just need some scripts to return 1 when fileconflict004 fails, and 0 when it pass. It could be pactest directly after patching it, or just a wrapper on pactest which greps "PASS = 1" or something like that.
Otherwise you can always manually use fileconflict004 as a test for doing a bisection using git, but then, it is probably faster to only do test before any commits related to fileconflict, rather than a blind testing.
Anyway, I still did a git bisect with manual testing for fun, and the result is funny :)
Here is a bisect script I have used with pacman in the past to do it automatically, I can't remember exactly which bug we were trying to track down but the ideas here are solid. You need to build, setup your test environment (if you are using pactest we really don't have to do much), run the test, and issue the correct exit code.
#!/bin/bash # make make # get setup correct sudo rm /var/lib/pacman/sync/{community,extra,pacman-git,testing,unstable}/.lastupdate # remove our temp file OUTFILE=/tmp/bisectout.txt [ -f $OUTFILE ] && rm $OUTFILE # don't want to actually do the upgrade, just see its output yes n | sudo ./src/pacman/pacman -Syu > $OUTFILE # if output contains # warning: licenses: local (2.4-1) is newer than core (2.3-1) # then it did the wrong thing grep -qF "warning: licenses: local (2.4-1) is newer than core (2.3-1)" $OUTFILE && exit 1 # we didn't find our text, good revision exit 0
"""" 584ffa6aef13d0933ad4930ab9cb70d3af2977ff is first bad commit commit 584ffa6aef13d0933ad4930ab9cb70d3af2977ff Author: Dan McGee dan@archlinux.org Date: Mon May 12 20:49:18 2008 -0500
Remove an outdated exception check in file conflict code
This has been around since at least pacman 2.9.8. Frugalware just dumped it in commit 113ec73bfcfdc, and deleting it here and running pactest shows that nothing that we have actually tested changes. If someone can pactest the edge case where this is needed, then show me the money.
Signed-off-by: Dan McGee dan@archlinux.org
:040000 040000 ea3e9f52732a5412f2e77e2a1060c3247636de7b a234d7e4ca725edc301f696640d897ce54dfb3b8 M lib """"
Hmmmmmm. So what do we need to do here? Put it back in? You guys break my code too much. :P
-Dan
On Tue, Jul 22, 2008 at 08:01:59PM -0500, Dan McGee dpmcgee@gmail.com wrote:
# get setup correct sudo rm /var/lib/pacman/sync/{community,extra,pacman-git,testing,unstable}/.lastupdate
Ouch, this is ugly. Why not using -Syy?
On Wed, Jul 23, 2008 at 3:01 AM, Miklos Vajna vmiklos@frugalware.org wrote:
On Tue, Jul 22, 2008 at 08:01:59PM -0500, Dan McGee dpmcgee@gmail.com wrote:
# get setup correct sudo rm /var/lib/pacman/sync/{community,extra,pacman-git,testing,unstable}/.lastupdate
Ouch, this is ugly. Why not using -Syy?
Are you really critiquing a bisect script? I don't know why I'm giving the time of day to this, but I'll bite.
This script was designed to find the problem that occurred *only* on -Syu and not -Su. Thus, a -Syy followed by and -Syu (which would update zero databases) would not trigger the problem.
commit f671147282e0f5c6a2e05c8cb7a0d5b72ef8cb61 Author: Xavier Chantry shiningxc@gmail.com Date: Mon May 12 18:56:14 2008 -0500
Fix rewinddir regression by cleaning up db_scan
Commit 046003844739416ff6d168dd2dec76490adb0727 caused a regression when rereading the pkgcache after updating the on-disk databases. A rewinddir call was errantly removed.
-Dan
On Wed, Jul 23, 2008 at 07:05:43AM -0500, Dan McGee dpmcgee@gmail.com wrote:
On Wed, Jul 23, 2008 at 3:01 AM, Miklos Vajna vmiklos@frugalware.org wrote:
On Tue, Jul 22, 2008 at 08:01:59PM -0500, Dan McGee dpmcgee@gmail.com wrote:
# get setup correct sudo rm /var/lib/pacman/sync/{community,extra,pacman-git,testing,unstable}/.lastupdate
Ouch, this is ugly. Why not using -Syy?
Are you really critiquing a bisect script? I don't know why I'm giving the time of day to this, but I'll bite.
Should I next time just ask, without stating the reason I ask? ;-)
This script was designed to find the problem that occurred *only* on -Syu and not -Su. Thus, a -Syy followed by and -Syu (which would update zero databases) would not trigger the problem.
I just thought -Syyu would work in that case, and wondered if there is a reason for doing so (read: learn from you as I suspected you know something that may be interesting).
Thanks.
On Wed, Jul 23, 2008 at 7:18 AM, Miklos Vajna vmiklos@frugalware.org wrote:
On Wed, Jul 23, 2008 at 07:05:43AM -0500, Dan McGee dpmcgee@gmail.com wrote:
On Wed, Jul 23, 2008 at 3:01 AM, Miklos Vajna vmiklos@frugalware.org wrote:
On Tue, Jul 22, 2008 at 08:01:59PM -0500, Dan McGee dpmcgee@gmail.com wrote:
# get setup correct sudo rm /var/lib/pacman/sync/{community,extra,pacman-git,testing,unstable}/.lastupdate
Ouch, this is ugly. Why not using -Syy?
Are you really critiquing a bisect script? I don't know why I'm giving the time of day to this, but I'll bite.
Should I next time just ask, without stating the reason I ask? ;-)
This script was designed to find the problem that occurred *only* on -Syu and not -Su. Thus, a -Syy followed by and -Syu (which would update zero databases) would not trigger the problem.
I just thought -Syyu would work in that case, and wondered if there is a reason for doing so (read: learn from you as I suspected you know something that may be interesting).
Ahh, and I forgot to mention that the core .lastupdate was not deleted. This was a key part of the equation if I remember right because of interactions with packages also in testing.
-Dan
On Wed, Jul 23, 2008 at 07:29:45AM -0500, Dan McGee dpmcgee@gmail.com wrote:
Ahh, and I forgot to mention that the core .lastupdate was not deleted. This was a key part of the equation if I remember right because of interactions with packages also in testing.
Thanks, that was the part I missed. ;-)
On Wed, Jul 23, 2008 at 3:01 AM, Dan McGee dpmcgee@gmail.com wrote:
Here is a bisect script I have used with pacman in the past to do it automatically, I can't remember exactly which bug we were trying to track down but the ideas here are solid. You need to build, setup your test environment (if you are using pactest we really don't have to do much), run the test, and issue the correct exit code.
So what about directly issuing the correct exit code in the pactest python script? Sounds like it would be a nice and helpful behavior, and it should be rather easy to do.
On Wed, Jul 23, 2008 at 3:18 AM, Xavier shiningxc@gmail.com wrote:
On Wed, Jul 23, 2008 at 3:01 AM, Dan McGee dpmcgee@gmail.com wrote:
Here is a bisect script I have used with pacman in the past to do it automatically, I can't remember exactly which bug we were trying to track down but the ideas here are solid. You need to build, setup your test environment (if you are using pactest we really don't have to do much), run the test, and issue the correct exit code.
So what about directly issuing the correct exit code in the pactest python script? Sounds like it would be a nice and helpful behavior, and it should be rather easy to do.
It makes 'make check' (and with that, 'make distcheck', etc.) fail if the pactest script returns anything but zero. I've had this change in a local branch for ages, but we need something else- a way to indicate known failure in pactest so we can (at least temporarily) continue on when our 5 or 6 known failures happen.
-Dan
c0f892be819a72370778c7b8a8604a268f43dd30 pactest/pactest.py | 9 ++++++- pactest/pmenv.py | 53 +++++++++++++++++++++++---------------------------- pactest/pmtest.py | 6 ++-- pactest/util.py | 5 +--- 4 files changed, 35 insertions(+), 38 deletions(-)
diff --git a/pactest/pactest.py b/pactest/pactest.py index 7cda255..b812c64 100755 --- a/pactest/pactest.py +++ b/pactest/pactest.py @@ -93,13 +93,18 @@ if __name__ == "__main__": env.pacman["valgrind"] = opts.valgrind env.pacman["manual-confirm"] = opts.manualconfirm
- if len(opts.testcases) == 0: + if opts.testcases is None or len(opts.testcases) == 0: print "no tests defined, nothing to do" + sys.exit(2) else: - for i in opts.testcases: env.addtest(i) + for i in opts.testcases: + env.addtest(i)
# run tests and print overall results env.run() env.results()
+ if len(env.failed) > 0: + sys.exit(1) + # vim: set ts=4 sw=4 et: diff --git a/pactest/pmenv.py b/pactest/pmenv.py old mode 100755 new mode 100644 index f2327f9..5960c0a --- a/pactest/pmenv.py +++ b/pactest/pmenv.py @@ -27,16 +27,19 @@ class pmenv: """Environment object """
+ pacman = { + "bin": "pacman", + "debug": 0, + "gdb": 0, + "valgrind": 0, + "nolog": 0 + } + testcases = [] + passed = [] + failed = [] + def __init__(self, root = "root"): self.root = os.path.abspath(root) - self.pacman = { - "bin": "pacman", - "debug": 0, - "gdb": 0, - "valgrind": 0, - "nolog": 0 - } - self.testcases = []
def __str__(self): return "root = %s\n" \ @@ -47,8 +50,8 @@ class pmenv: """ """ if not os.path.isfile(testcase): - err("file %s not found" % testcase) - return + raise IOError("test file %s not found" % testcase) + test = pmtest.pmtest(testcase, self.root) self.testcases.append(test)
@@ -76,24 +79,16 @@ class pmenv: t.check() print "==> Test result" if t.result["fail"] == 0: + self.passed.append(t) print "\tPASS" else: + self.failed.append(t) print "\tFAIL" print
def results(self): """ """ - passed = 0 - tpassed = [] - tfailed = [] - for test in self.testcases: - fail = test.result["fail"] - if fail == 0: - passed += 1 - tpassed.append(test) - else: - tfailed.append(test)
def _printtest(t): success = test.result["success"] @@ -114,20 +109,20 @@ class pmenv: print "=========="*8 print "Results" print "----------"*8 - for test in tpassed: _printtest(test) + for test in self.passed: _printtest(test) print "----------"*8 - for test in tfailed: _printtest(test) + for test in self.failed: _printtest(test) print "----------"*8
- total = len(self.testcases) - failed = total - passed + passes = len(self.passed) + fails = len(self.failed) + total = passes + fails print "TOTAL = %3u" % total - if total: - print "PASS = %3u (%6.2f%%)" % (passed, float(passed) * 100 / total) - print "FAIL = %3u (%6.2f%%)" % (failed, float(failed) * 100 / total) + print "PASS = %3u (%6.2f%%)" % (passes, float(passes) * 100 / total) + print "FAIL = %3u (%6.2f%%)" % (fails, float(fails) * 100 / total) print ""
if __name__ == "__main__": - env = pmenv("/tmp") - print env + pass + # vim: set ts=4 sw=4 et: diff --git a/pactest/pmtest.py b/pactest/pmtest.py old mode 100755 new mode 100644 index e8f6fa8..a34efda --- a/pactest/pmtest.py +++ b/pactest/pmtest.py @@ -94,7 +94,7 @@ class pmtest: if os.path.isfile(self.name): execfile(self.name) else: - err("file %s does not exist!" % self.name) + raise IOError("file %s does not exist!" % self.name)
def generate(self): """ @@ -267,6 +267,6 @@ class pmtest:
if __name__ == "__main__": - test = pmtest("test1", "./root") - print test + pass + # vim: set ts=4 sw=4 et: diff --git a/pactest/util.py b/pactest/util.py old mode 100755 new mode 100644 index c6d5a59..db9560f --- a/pactest/util.py +++ b/pactest/util.py @@ -43,10 +43,6 @@ LOGFILE = "var/log/pactest.log"
verbose = 0
-def err(msg): - print "error: " + msg - sys.exit(1) - def vprint(msg): if verbose: print msg @@ -273,4 +269,5 @@ def mkdir(dir):
if __name__ == "__main__": pass + # vim: set ts=4 sw=4 et:
On Wed, Jul 23, 2008 at 2:01 PM, Dan McGee dpmcgee@gmail.com wrote:
It makes 'make check' (and with that, 'make distcheck', etc.) fail if the pactest script returns anything but zero. I've had this change in a local branch for ages, but we need something else- a way to indicate known failure in pactest so we can (at least temporarily) continue on when our 5 or 6 known failures happen.
Is it possible to just go ahead with that for now or not? Would it be a problem if make check failed, even if they are all known failures? I am afraid I don't know much about make check / make distcheck / etc, so I am asking.
I think it would be neat to be able to write a very simple bisect script checking for a specific pactest failure, that is, being able to just put pactest/pactest.py -p src/pacman/pacman -t /path/to/pactest in the bisect script, and it would simply work.
On Wed, Jul 23, 2008 at 7:42 AM, Xavier shiningxc@gmail.com wrote:
On Wed, Jul 23, 2008 at 2:01 PM, Dan McGee dpmcgee@gmail.com wrote:
It makes 'make check' (and with that, 'make distcheck', etc.) fail if the pactest script returns anything but zero. I've had this change in a local branch for ages, but we need something else- a way to indicate known failure in pactest so we can (at least temporarily) continue on when our 5 or 6 known failures happen.
Is it possible to just go ahead with that for now or not? Would it be a problem if make check failed, even if they are all known failures? I am afraid I don't know much about make check / make distcheck / etc, so I am asking.
I'd really rather not break make distcheck- its one less thing for me to worry about when packaging everything up if that works.
I think it would be neat to be able to write a very simple bisect script checking for a specific pactest failure, that is, being able to just put pactest/pactest.py -p src/pacman/pacman -t /path/to/pactest in the bisect script, and it would simply work.
I was thinking of adding some kind of "self.knownoutcome" flag to pactest, and have that basically suppress the return code incrementing if it was set to "fail" or something. (or just self.knownfailure=true.) Does that make sense to anyone? The problem is right now we have no way of distinguishing from fails like fileconflict 001 & 002 (which we know will fail from now until they are fixed) from fails that pop up after a patch has been applied. The second ones are the regressions and the ones we really care about; the first are not quite as important in a normal run of pactest.
-Dan
On Wed, Jul 23, 2008 at 2:51 PM, Dan McGee dpmcgee@gmail.com wrote:
I was thinking of adding some kind of "self.knownoutcome" flag to pactest, and have that basically suppress the return code incrementing if it was set to "fail" or something. (or just self.knownfailure=true.) Does that make sense to anyone? The problem is right now we have no way of distinguishing from fails like fileconflict 001 & 002 (which we know will fail from now until they are fixed) from fails that pop up after a patch has been applied. The second ones are the regressions and the ones we really care about; the first are not quite as important in a normal run of pactest.
I agree, a way to distinguish them would be very helpful.
I was thinking of adding some kind of "self.knownoutcome" flag to pactest, and have that basically suppress the return code incrementing if it was set to "fail" or something. (or just self.knownfailure=true.) Does that make sense to anyone? The problem is right now we have no way of distinguishing from fails like fileconflict 001 & 002 (which we know will fail from now until they are fixed) from fails that pop up after a patch has been applied. The second ones are the regressions and the ones we really care about; the first are not quite as important in a normal run of pactest.
Hm. This inspired me to ask the following question: Shouldn't we document somewhere the "known issues"?
Bye
On Wed, Jul 23, 2008 at 3:05 PM, Nagy Gabor ngaba@bibl.u-szeged.hu wrote:
I was thinking of adding some kind of "self.knownoutcome" flag to pactest, and have that basically suppress the return code incrementing if it was set to "fail" or something. (or just self.knownfailure=true.) Does that make sense to anyone? The problem is right now we have no way of distinguishing from fails like fileconflict 001 & 002 (which we know will fail from now until they are fixed) from fails that pop up after a patch has been applied. The second ones are the regressions and the ones we really care about; the first are not quite as important in a normal run of pactest.
Hm. This inspired me to ask the following question: Shouldn't we document somewhere the "known issues"?
I agree, behind all these known pactest failures, there is usually a longer explanation on the ML or on the bug tracker. But then, a simple link inside the pactest would be enough.
On Wed, Jul 23, 2008 at 07:51:38AM -0500, Dan McGee dpmcgee@gmail.com wrote:
I was thinking of adding some kind of "self.knownoutcome" flag to pactest, and have that basically suppress the return code incrementing if it was set to "fail" or something. (or just self.knownfailure=true.) Does that make sense to anyone?
Or just self.expectko = True (maybe .expectfailure). Return code is just one rule, others may be known to be broken as well.
On Tue, Jul 22, 2008 at 8:20 AM, Xavier shiningxc@gmail.com wrote:
Anyway, I still did a git bisect with manual testing for fun, and the result is funny :)
"""" 584ffa6aef13d0933ad4930ab9cb70d3af2977ff is first bad commit commit 584ffa6aef13d0933ad4930ab9cb70d3af2977ff Author: Dan McGee dan@archlinux.org Date: Mon May 12 20:49:18 2008 -0500
Remove an outdated exception check in file conflict code
This has been around since at least pacman 2.9.8. Frugalware just dumped it in commit 113ec73bfcfdc, and deleting it here and running pactest shows that nothing that we have actually tested changes. If someone can pactest the edge case where this is needed, then show me the money.
Signed-off-by: Dan McGee dan@archlinux.org
:040000 040000 ea3e9f52732a5412f2e77e2a1060c3247636de7b a234d7e4ca725edc301f696640d897ce54dfb3b8 M lib """"
Well here you go, the pactest is called fileconflict004, courtesy of Nagy ;)
And if we want to toss some fireworks around, reverting this commit would fix 004 as well (at the expense of 003). :P
commit a71f4c4c6a7775503b29ab856f8a5ce2801f2967 Author: Chantry Xavier shiningxc@gmail.com Date: Sun Jan 6 10:59:41 2008 +0100
conflict.c : fix for FS#8156, detect conflict between symlink and dir.
Dan McGee wrote:
And if we want to toss some fireworks around, reverting this commit would fix 004 as well (at the expense of 003). :P
commit a71f4c4c6a7775503b29ab856f8a5ce2801f2967 Author: Chantry Xavier shiningxc@gmail.com Date: Sun Jan 6 10:59:41 2008 +0100
conflict.c : fix for FS#8156, detect conflict between symlink and dir.
Indeed, that is what I said in the beginning, it seemed logical to me that this patch fixed fileconflict003 and broke fileconflict004, but it did not back then, because that strange code you removed was still there. So removing it was not a bad thing since it makes the code clearer and the behavior of my patch predictable. Anyway, I think it is always important to understand what happened, but it doesn't always help getting things fixed.
This patch makes both fileconflict003 and fileconflict004 pactests successfully pass. It is a rehash of the check was removed in commit 584ffa6aef13d0933ad4930ab9cb70d3af2977ff, but done slightly differently. I'm still not very fond of this code, and it would be much better if we knew more about the filelist than a listing of paths- we have no real concept of whether a to-be-installed file is a symlink, etc.
Issue: http://archlinux.org/pipermail/pacman-dev/2008-July/012465.html
Signed-off-by: Dan McGee dan@archlinux.org --- lib/libalpm/conflict.c | 30 ++++++++++++++++++++++-------- 1 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index a8bcdd5..f0e1116 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -434,14 +434,28 @@ alpm_list_t *_alpm_db_find_fileconflicts(pmdb_t *db, pmtrans_t *trans, char *roo } stat(path, &sbuf);
- if(path[strlen(path)-1] == '/') { - if(S_ISDIR(lsbuf.st_mode)) { - _alpm_log(PM_LOG_DEBUG, "%s is a directory, not a conflict\n", path); - skip_conflict = 1; - } else if(S_ISLNK(lsbuf.st_mode) && S_ISDIR(sbuf.st_mode)) { - _alpm_log(PM_LOG_DEBUG, - "%s is a symlink to a dir, hopefully not a conflict\n", path); - skip_conflict = 1; + if(path[strlen(path)-1] == '/' && S_ISDIR(lsbuf.st_mode)) { + _alpm_log(PM_LOG_DEBUG, "%s is a directory, not a conflict\n", path); + skip_conflict = 1; + } else if(S_ISLNK(lsbuf.st_mode) && S_ISDIR(sbuf.st_mode)) { + _alpm_log(PM_LOG_DEBUG, + "%s is a symlink to a dir, hopefully not a conflict\n", path); + skip_conflict = 1; + } else if(S_ISDIR(sbuf.st_mode)) { + if(dbpkg) { + /* Make sure the possible conflict is not a symlink that points + * to a path in the old package. */ + struct stat pkgbuf; + char str[PATH_MAX+1]; + for(k = alpm_pkg_get_files(dbpkg); k; k = k->next) { + snprintf(str, PATH_MAX, "%s%s", root, (char*)k->data); + if(!_alpm_lstat(str, &pkgbuf) && lsbuf.st_ino == pkgbuf.st_ino) { + skip_conflict = 1; + _alpm_log(PM_LOG_DEBUG, + "%s is a symlink to a dir in previous package\n", path); + break; + } + } } } if(!skip_conflict) {
On Wed, Jul 23, 2008 at 4:14 AM, Dan McGee dan@archlinux.org wrote:
This patch makes both fileconflict003 and fileconflict004 pactests successfully pass. It is a rehash of the check was removed in commit 584ffa6aef13d0933ad4930ab9cb70d3af2977ff, but done slightly differently. I'm still not very fond of this code, and it would be much better if we knew more about the filelist than a listing of paths- we have no real concept of whether a to-be-installed file is a symlink, etc.
Issue: http://archlinux.org/pipermail/pacman-dev/2008-July/012465.html
Signed-off-by: Dan McGee dan@archlinux.org
lib/libalpm/conflict.c | 30 ++++++++++++++++++++++-------- 1 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index a8bcdd5..f0e1116 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -434,14 +434,28 @@ alpm_list_t *_alpm_db_find_fileconflicts(pmdb_t *db, pmtrans_t *trans, char *roo } stat(path, &sbuf);
if(path[strlen(path)-1] == '/') {if(S_ISDIR(lsbuf.st_mode)) {_alpm_log(PM_LOG_DEBUG, "%s is a directory, not a conflict\n", path);skip_conflict = 1;} else if(S_ISLNK(lsbuf.st_mode) && S_ISDIR(sbuf.st_mode)) {_alpm_log(PM_LOG_DEBUG,"%s is a symlink to a dir, hopefully not a conflict\n", path);skip_conflict = 1;
if(path[strlen(path)-1] == '/' && S_ISDIR(lsbuf.st_mode)) {_alpm_log(PM_LOG_DEBUG, "%s is a directory, not a conflict\n", path);skip_conflict = 1;} else if(S_ISLNK(lsbuf.st_mode) && S_ISDIR(sbuf.st_mode)) {_alpm_log(PM_LOG_DEBUG,"%s is a symlink to a dir, hopefully not a conflict\n", path);skip_conflict = 1;} else if(S_ISDIR(sbuf.st_mode)) {if(dbpkg) {/* Make sure the possible conflict is not a symlink that points* to a path in the old package. */struct stat pkgbuf;char str[PATH_MAX+1];for(k = alpm_pkg_get_files(dbpkg); k; k = k->next) {snprintf(str, PATH_MAX, "%s%s", root, (char*)k->data);if(!_alpm_lstat(str, &pkgbuf) && lsbuf.st_ino == pkgbuf.st_ino) {skip_conflict = 1;_alpm_log(PM_LOG_DEBUG,"%s is a symlink to a dir in previous package\n", path);break;}} } } if(!skip_conflict) {-- 1.5.6.4
Well, I am unable to understand that code, but maybe Nagy did, since he was able to provide a (fileconflict005 ?) pactest which fails after this patch : http://archlinux.org/pipermail/pacman-dev/2008-July/012522.html That pactest also triggers this "%s is a symlink to a dir in previous package\n" message which looks totally bogus.
"/path/to/pacman/root/test is a symlink to a dir in previous package"
I don't see how this reflects the situation of "fileconflict005" at all. In the new package, test is a symlink to a new directory test2. and test2 did not exist in any previous package. However, test existed in previous packages, as a directory.
Anyway, here are the different ways I propose to deal with this issue : 1) choose that fileconflict003 is more important than 004, and only supports that situation (current 3.2 code) 2) choose that 004 is more important than 003, so slightly edit the code to get back to this old behavior (of 3.0?) 3) we decide that we need to support both, in that case we go the way that Nagy suggested in the beginning : http://archlinux.org/pipermail/pacman-dev/2008-July/012470.html
In the first 2 cases, the code will remain (relatively) simple and understandable. In the third one, it will get more complex, but it will still be logical and handle all the cases correctly.
Well, I am unable to understand that code, but maybe Nagy did, since he was able to provide a (fileconflict005 ?) pactest which fails after this patch : http://archlinux.org/pipermail/pacman-dev/2008-July/012522.html That pactest also triggers this "%s is a symlink to a dir in previous package\n" message which looks totally bogus.
OK. That ugly (not portable?) inode hack catches this case _accidentally_ which can be replaced by a much "cheaper" test, if needed. So what is happening here? See commit 584ffa6aef13d0933ad4930ab9cb70d3af2977ff. First of all, that part checks whether the _local file_ is a symlink pointing to a to-be-removed file (dir?). I don't see why this case should be handled at all. Clearly, this is not that case, we have a local conflicting _directory_, not symlink, but the code believes that it is that case. To be short: S_ISLNK(lsbuf.st_mode) check is missing from the removed code-part. This leads to the false decision which is good from fileconflict004.py viewpoint. See the decision part: if(!_alpm_lstat(str, &pkgbuf) && lsbuf.st_ino == pkgbuf.st_ino)... /* lsbuf represents the conflicting file, pkgbuf loops over the 'files' of local package */ Since there is no symlink here, _alpm_lstat is equivalent with stat (the code _assumed_ symlink here). So this part will simply detect that our conflicting directory belongs to the local package 'files' list. (Which doesn't mean that it will be removed, see fileconflict005.py).
So if you want to restore the old behaviour, this code-part can be replaced with an easy "find directory-name in localpkg's filelist" search (maybe tuned with realpath).
"/path/to/pacman/root/test is a symlink to a dir in previous package"
I don't see how this reflects the situation of "fileconflict005" at all. In the new package, test is a symlink to a new directory test2. and test2 did not exist in any previous package. However, test existed in previous packages, as a directory.
Anyway, here are the different ways I propose to deal with this issue : 1) choose that fileconflict003 is more important than 004, and only supports that situation (current 3.2 code) 2) choose that 004 is more important than 003, so slightly edit the code to get back to this old behavior (of 3.0?) 3) we decide that we need to support both, in that case we go the way that Nagy suggested in the beginning : http://archlinux.org/pipermail/pacman-dev/2008-July/012470.html
I am considering a "cheaper" solution now, only check the old version of package (not the whole target list). But that cannot
My opinion about fileconflict check, that it is really fast, but it has some bugs (most important: trans001.py). I think we should decide if we want to implement FS#8585 *soon* or not, that would solve most of these issues. If we want, I don't think we should waste too much effort to this to-be-removed fileconflict check part.
Bye
if(!_alpm_lstat(str, &pkgbuf) && lsbuf.st_ino == pkgbuf.st_ino)... /* lsbuf represents the conflicting file, pkgbuf loops over the 'files' of local package */ Since there is no symlink here, _alpm_lstat is equivalent with stat (the code _assumed_ symlink here).
Not the _alpm_lstat in this line is interesting here, but the alpm_lstat which filled lsbuf earlier.
So if you want to restore the old behaviour, this code-part can be replaced with an easy "find directory-name in localpkg's filelist" search (maybe tuned with realpath).
s/directoy-name/directory-path/
I am considering a "cheaper" solution now, only check the old version of package (not the whole target list). But that cannot
s/But that cannot//
On Wed, Jul 23, 2008 at 1:58 PM, Nagy Gabor ngaba@bibl.u-szeged.hu wrote:
My opinion about fileconflict check, that it is really fast, but it has some bugs (most important: trans001.py). I think we should decide if we want to implement FS#8585 *soon* or not, that would solve most of these issues. If we want, I don't think we should waste too much effort to this to-be-removed fileconflict check part.
I share your opinion. But I can't make that decision.
On Wed, Jul 23, 2008 at 7:30 AM, Xavier shiningxc@gmail.com wrote:
On Wed, Jul 23, 2008 at 1:58 PM, Nagy Gabor ngaba@bibl.u-szeged.hu wrote:
My opinion about fileconflict check, that it is really fast, but it has some bugs (most important: trans001.py). I think we should decide if we want to implement FS#8585 *soon* or not, that would solve most of these issues. If we want, I don't think we should waste too much effort to this to-be-removed fileconflict check part.
I share your opinion. But I can't make that decision.
I think we should just put some jury-rigged, hopefully passes both 003 and 004 code in here and really attempt to fix this on the larger scale post-3.2. Xavier, would a revert of my "useless check" patch solve the problem for now?
-Dan
On Wed, Jul 23, 2008 at 2:53 PM, Dan McGee dpmcgee@gmail.com wrote:
I think we should just put some jury-rigged, hopefully passes both 003 and 004 code in here and really attempt to fix this on the larger scale post-3.2. Xavier, would a revert of my "useless check" patch solve the problem for now?
Nagy clearly said that part is broken and does not even do what it is supposed to do. Besides, while 003 indeed passes, the similar 005 test case does not. And finally, all the quick hacks I did like this during the 3.1 releases, "to be cleaned up for 3.2", are all still here.
The only options I can support are the following ones : 1) doing nothing. I was able to work around the xulrunner case just fine with pacman -Rd xulrunner && pacman -S xulrunner 2) nagy solution : check ownerships of all files inside the conflicting dir 3) roll back system : FS#8585
Note that all these options can be followed, in the same order (first 1, then later 2, then even later 3).
On Wed, Jul 23, 2008 at 8:14 AM, Xavier shiningxc@gmail.com wrote:
On Wed, Jul 23, 2008 at 2:53 PM, Dan McGee dpmcgee@gmail.com wrote:
I think we should just put some jury-rigged, hopefully passes both 003 and 004 code in here and really attempt to fix this on the larger scale post-3.2. Xavier, would a revert of my "useless check" patch solve the problem for now?
Nagy clearly said that part is broken and does not even do what it is supposed to do. Besides, while 003 indeed passes, the similar 005 test case does not. And finally, all the quick hacks I did like this during the 3.1 releases, "to be cleaned up for 3.2", are all still here.
The only options I can support are the following ones :
- doing nothing. I was able to work around the xulrunner case just
fine with pacman -Rd xulrunner && pacman -S xulrunner 2) nagy solution : check ownerships of all files inside the conflicting dir 3) roll back system : FS#8585
Note that all these options can be followed, in the same order (first 1, then later 2, then even later 3).
OK, this is our blocker. I plan on pushing the rest of my stuff out tonight and that will be our 3.2.0 release if possible. What do we do here? I'd like a statement of exactly what to do (or nothing at all), or a patch to implement a short-term fix.
-Dan
Dan McGee wrote:
On Wed, Jul 23, 2008 at 8:14 AM, Xavier shiningxc@gmail.com wrote:
On Wed, Jul 23, 2008 at 2:53 PM, Dan McGee dpmcgee@gmail.com wrote:
I think we should just put some jury-rigged, hopefully passes both 003 and 004 code in here and really attempt to fix this on the larger scale post-3.2. Xavier, would a revert of my "useless check" patch solve the problem for now?
Nagy clearly said that part is broken and does not even do what it is supposed to do. Besides, while 003 indeed passes, the similar 005 test case does not. And finally, all the quick hacks I did like this during the 3.1 releases, "to be cleaned up for 3.2", are all still here.
The only options I can support are the following ones :
- doing nothing. I was able to work around the xulrunner case just
fine with pacman -Rd xulrunner && pacman -S xulrunner 2) nagy solution : check ownerships of all files inside the conflicting dir 3) roll back system : FS#8585
Note that all these options can be followed, in the same order (first 1, then later 2, then even later 3).
OK, this is our blocker. I plan on pushing the rest of my stuff out tonight and that will be our 3.2.0 release if possible. What do we do here? I'd like a statement of exactly what to do (or nothing at all), or a patch to implement a short-term fix.
I'm happy doing nothing but a short-term fix to get this back to at least not being a regression on 3.1 would be best. If it can pass more of the fileconflict pactests, then good but from what I understand about this bug, a complete fix is too big of a change at this time.
Allan
On Wed, Jul 23, 2008 at 8:14 AM, Xavier shiningxc@gmail.com wrote:
On Wed, Jul 23, 2008 at 2:53 PM, Dan McGee dpmcgee@gmail.com wrote:
I think we should just put some jury-rigged, hopefully passes both 003 and 004 code in here and really attempt to fix this on the larger scale post-3.2. Xavier, would a revert of my "useless check" patch solve the problem for now?
Nagy clearly said that part is broken and does not even do what it is supposed to do. Besides, while 003 indeed passes, the similar 005 test case does not. And finally, all the quick hacks I did like this during the 3.1 releases, "to be cleaned up for 3.2", are all still here.
The only options I can support are the following ones :
- doing nothing. I was able to work around the xulrunner case just
fine with pacman -Rd xulrunner && pacman -S xulrunner 2) nagy solution : check ownerships of all files inside the conflicting dir 3) roll back system : FS#8585
Note that all these options can be followed, in the same order (first 1, then later 2, then even later 3).
OK, this is our blocker. I plan on pushing the rest of my stuff out tonight and that will be our 3.2.0 release if possible. What do we do here? I'd like a statement of exactly what to do (or nothing at all), or a patch to implement a short-term fix.
-Dan
IMHO nothing atm. Point 2) seems easy, but it is complicated: we must ensure that just after remove_upgrade part of xulrunner, we have no existing conflicting dir, checking ownerships inside the dir is not enough, new appearing files (due to the upgrade of other packages) can happen.
If you want to restore our old negligent "eye-candy" behaviour, you can do the following (pseudo-code): if(isdir(local_conflicting_stuff) && alpm_list_find_str(oldpkg->files,local_conflicting_stuff) { continue; /*no conflict*/ }
On Fri, Jul 25, 2008 at 10:27 AM, Nagy Gabor ngaba@bibl.u-szeged.hu wrote:
If you want to restore our old negligent "eye-candy" behaviour, you can do the following (pseudo-code): if(isdir(local_conflicting_stuff) && alpm_list_find_str(oldpkg->files,local_conflicting_stuff) { continue; /*no conflict*/ }
I would say go ahead with the current code for the 3.2 release. If it turns out to be really problematic, we can always use the above code for 3.2.1. (again, it is a matter of choosing which case is more important between fileconflict003 and fileconflict004)
If you want to restore our old negligent "eye-candy" behaviour, you can do the following (pseudo-code): if(isdir(local_conflicting_stuff) && alpm_list_find_str(oldpkg->files,local_conflicting_stuff) { continue; /*no conflict*/ }
I would say go ahead with the current code for the 3.2 release. If it turns out to be really problematic, we can always use the above code for 3.2.1. (again, it is a matter of choosing which case is more important between fileconflict003 and fileconflict004)
I might have not clear, I also vote for this (I just said above, that forget that ugly i-node hack part.) I tell you why: The current 3.2 code is _safer_. With the old one, we had a failing fileconflict005.py pactest (maybe we should put it to git), which is problematic, because your installed package will be broken. Currently the code is "too safe", this is the problem, but this causes just "annoyance" and can be fixed with -Sf.
Bye
On Fri, Jul 25, 2008 at 2:11 PM, Nagy Gabor ngaba@bibl.u-szeged.hu wrote:
I might have not clear, I also vote for this (I just said above, that forget that ugly i-node hack part.) I tell you why: The current 3.2 code is _safer_. With the old one, we had a failing fileconflict005.py pactest (maybe we should put it to git)
If that pactest is interesting, just create a commit in your git tree, it is yours :)
On Fri, Jul 25, 2008 at 2:11 PM, Nagy Gaborngaba@bibl.u-szeged.hu wrote:
If you want to restore our old negligent "eye-candy" behaviour, you can do the following (pseudo-code): if(isdir(local_conflicting_stuff) && alpm_list_find_str(oldpkg->files,local_conflicting_stuff) { continue; /*no conflict*/ }
I would say go ahead with the current code for the 3.2 release. If it turns out to be really problematic, we can always use the above code for 3.2.1. (again, it is a matter of choosing which case is more important between fileconflict003 and fileconflict004)
I might have not clear, I also vote for this (I just said above, that forget that ugly i-node hack part.) I tell you why: The current 3.2 code is _safer_. With the old one, we had a failing fileconflict005.py pactest (maybe we should put it to git), which is problematic, because your installed package will be broken. Currently the code is "too safe", this is the problem, but this causes just "annoyance" and can be fixed with -Sf.
I am not sure what to do. It seems the situation of fileconflict004 happens regularly, see for example : http://www.archlinux.org/news/444/ And I believe there were also other cases since 3.2 was released.
So maybe we should implement the pseudo-code that Nagy proposed above. I have a simple patch locally which works.
Then fileconflict005 fails, but this is probably a less common situation. And pacman will just fail to detect the file conflict, and skip the extraction of a file or a symlink (because a non-empty directory exists). Though, I don't understand why pacman shows an error in case of a file, and nothing (only a debug message) in case of a symlink (case of fileconflict005). So I would suggest displaying an error in case of a symlink as well, to have an output as follows:
checking package integrity... checking for file conflicts... upgrading pkg1... error: extract: skipping symlink extraction of test error: problem occurred while upgrading pkg1
On Fri, Jul 17, 2009 at 3:10 PM, Xaviershiningxc@gmail.com wrote:
I am not sure what to do. It seems the situation of fileconflict004 happens regularly, see for example : http://www.archlinux.org/news/444/ And I believe there were also other cases since 3.2 was released.
So maybe we should implement the pseudo-code that Nagy proposed above. I have a simple patch locally which works.
Then fileconflict005 fails, but this is probably a less common situation. And pacman will just fail to detect the file conflict, and skip the extraction of a file or a symlink (because a non-empty directory exists). Though, I don't understand why pacman shows an error in case of a file, and nothing (only a debug message) in case of a symlink (case of fileconflict005). So I would suggest displaying an error in case of a symlink as well, to have an output as follows:
checking package integrity... checking for file conflicts... upgrading pkg1... error: extract: skipping symlink extraction of test error: problem occurred while upgrading pkg1
I am not completely sure it is the right move, but in any cases, it shouldn't hurt much as this is just the pre-3.2 behavior. So attached a patch.
Idézet Xavier shiningxc@gmail.com:
On Fri, Jul 25, 2008 at 2:11 PM, Nagy Gaborngaba@bibl.u-szeged.hu wrote:
If you want to restore our old negligent "eye-candy" behaviour, you can do the following (pseudo-code): if(isdir(local_conflicting_stuff) && alpm_list_find_str(oldpkg->files,local_conflicting_stuff) { continue; /*no conflict*/ }
I would say go ahead with the current code for the 3.2 release. If it turns out to be really problematic, we can always use the above code for 3.2.1. (again, it is a matter of choosing which case is more important between fileconflict003 and fileconflict004)
I might have not clear, I also vote for this (I just said above, that forget that ugly i-node hack part.) I tell you why: The current 3.2 code is _safer_. With the old one, we had a failing fileconflict005.py pactest (maybe we should put it to git), which is problematic, because your installed package will be broken. Currently the code is "too safe", this is the problem, but this causes just "annoyance" and can be fixed with -Sf.
I am not sure what to do. It seems the situation of fileconflict004 happens regularly, see for example : http://www.archlinux.org/news/444/ And I believe there were also other cases since 3.2 was released.
So maybe we should implement the pseudo-code that Nagy proposed above. I have a simple patch locally which works.
Well, I would be happy with a bit more careful code. I think we need a "fast" check (no realpath etc.) to ensure that all files in the dir will be removed in oldpkg removal. If all files of the conflicting dir will be removed by oldpkg, then go ahead. (Don't check other packages, only the old version of the package in question, this is important to eliminate the "file relocation" hack.) Even this doesn't ensure that the directory will be removed (.pacsave, NoUpgrade etc), but this is more than nothing. Unfortunately here we need a recursive stuff...
Then fileconflict005 fails, but this is probably a less common situation. And pacman will just fail to detect the file conflict, and skip the extraction of a file or a symlink (because a non-empty directory exists). Though, I don't understand why pacman shows an error in case of a file, and nothing (only a debug message) in case of a symlink (case of fileconflict005). So I would suggest displaying an error in case of a symlink as well, to have an output as follows:
------------------------------------------------------ SZTE Egyetemi Konyvtar - http://www.bibl.u-szeged.hu This message was sent using IMP: http://horde.org/imp/
When one package wants to replace a directory by a file, we check that all files in that directory were owned by that package.
Additionally pacman can be more verbose when the extraction of the symlink (or file) fails. The patch to add.c looks more complex than it is, I just moved and reindented code to handle cases 10 and 11 together.
Signed-off-by: Xavier Chantry shiningxc@gmail.com --- lib/libalpm/add.c | 48 +++++++++++++++----------------- lib/libalpm/conflict.c | 56 ++++++++++++++++++++++++++++++++++++++ pactest/tests/fileconflict004.py | 2 - pactest/tests/fileconflict005.py | 1 - pactest/tests/fileconflict006.py | 24 ++++++++++++++++ 5 files changed, 102 insertions(+), 29 deletions(-) create mode 100644 pactest/tests/fileconflict006.py
diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 4bd52de..ddbcfee 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -353,28 +353,30 @@ static int extract_single_file(struct archive *archive, if(_alpm_lstat(filename, &lsbuf) != 0 || stat(filename, &sbuf) != 0) { /* cases 1,2,3: couldn't stat an existing file, skip all backup checks */ } else { - if(S_ISDIR(lsbuf.st_mode) && S_ISDIR(entrymode)) { - /* case 12: existing dir, ignore it */ - if(lsbuf.st_mode != entrymode) { - /* if filesystem perms are different than pkg perms, warn user */ - int mask = 07777; - _alpm_log(PM_LOG_WARNING, _("directory permissions differ on %s\n" - "filesystem: %o package: %o\n"), entryname, lsbuf.st_mode & mask, - entrymode & mask); - alpm_logaction("warning: directory permissions differ on %s\n" + if(S_ISDIR(lsbuf.st_mode)) { + if(S_ISDIR(entrymode)) { + /* case 12: existing dir, ignore it */ + if(lsbuf.st_mode != entrymode) { + /* if filesystem perms are different than pkg perms, warn user */ + int mask = 07777; + _alpm_log(PM_LOG_WARNING, _("directory permissions differ on %s\n" + "filesystem: %o package: %o\n"), entryname, lsbuf.st_mode & mask, + entrymode & mask); + alpm_logaction("warning: directory permissions differ on %s\n" "filesystem: %o package: %o\n", entryname, lsbuf.st_mode & mask, - entrymode & mask); + entrymode & mask); + } + _alpm_log(PM_LOG_DEBUG, "extract: skipping dir extraction of %s\n", + entryname); + archive_read_data_skip(archive); + return(0); + } else { + /* case 10/11: trying to overwrite dir with file/symlink, don't allow it */ + _alpm_log(PM_LOG_ERROR, _("extract: not overwriting dir with file %s\n"), + entryname); + archive_read_data_skip(archive); + return(1); } - _alpm_log(PM_LOG_DEBUG, "extract: skipping dir extraction of %s\n", - entryname); - archive_read_data_skip(archive); - return(0); - } else if(S_ISDIR(lsbuf.st_mode) && S_ISLNK(entrymode)) { - /* case 11: existing dir, symlink in package, ignore it */ - _alpm_log(PM_LOG_DEBUG, "extract: skipping symlink extraction of %s\n", - entryname); - archive_read_data_skip(archive); - return(0); } else if(S_ISLNK(lsbuf.st_mode) && S_ISDIR(entrymode)) { /* case 9: existing symlink, dir in package */ if(S_ISDIR(sbuf.st_mode)) { @@ -390,12 +392,6 @@ static int extract_single_file(struct archive *archive, archive_read_data_skip(archive); return(1); } - } else if(S_ISDIR(lsbuf.st_mode) && S_ISREG(entrymode)) { - /* case 10: trying to overwrite dir tree with file, don't allow it */ - _alpm_log(PM_LOG_ERROR, _("extract: not overwriting dir with file %s\n"), - entryname); - archive_read_data_skip(archive); - return(1); } else if(S_ISREG(lsbuf.st_mode) && S_ISDIR(entrymode)) { /* case 6: trying to overwrite file with dir */ _alpm_log(PM_LOG_DEBUG, "extract: overwriting file with dir %s\n", diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index db1656f..9cc46d2 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -29,6 +29,7 @@ #include <string.h> #include <limits.h> #include <sys/stat.h> +#include <dirent.h>
/* libalpm */ #include "conflict.h" @@ -348,6 +349,50 @@ void _alpm_fileconflict_free(pmfileconflict_t *conflict) FREE(conflict); }
+static int dir_belongsto_pkg(char *dirpath, pmpkg_t *pkg) +{ + struct dirent *ent = NULL; + struct stat sbuf; + char path[PATH_MAX]; + char abspath[PATH_MAX]; + DIR *dir; + + snprintf(abspath, PATH_MAX, "%s%s", handle->root, dirpath); + dir = opendir(abspath); + if(dir == NULL) { + return(1); + } + while((ent = readdir(dir)) != NULL) { + const char *name = ent->d_name; + + if(strcmp(name, ".") == 0 || strcmp(name, "..") == 0) { + continue; + } + snprintf(path, PATH_MAX, "%s/%s", dirpath, name); + snprintf(abspath, PATH_MAX, "%s%s", handle->root, path); + if(stat(abspath, &sbuf) != 0) { + continue; + } + if(S_ISDIR(sbuf.st_mode)) { + if(dir_belongsto_pkg(path, pkg)) { + continue; + } else { + closedir(dir); + return(0); + } + } else { + if(alpm_list_find_str(alpm_pkg_get_files(pkg),path)) { + continue; + } else { + closedir(dir); + return(0); + } + } + } + closedir(dir); + return(1); +} + /* Find file conflicts that may occur during the transaction with two checks: * 1: check every target against every target * 2: check every target against the filesystem */ @@ -474,6 +519,17 @@ alpm_list_t *_alpm_db_find_fileconflicts(pmdb_t *db, pmtrans_t *trans, } }
+ if(S_ISDIR(lsbuf.st_mode) && dbpkg) { + char *dir = malloc(strlen(filestr) + 2); + sprintf(dir, "%s/", filestr); + if(alpm_list_find_str(alpm_pkg_get_files(dbpkg),dir)) { + _alpm_log(PM_LOG_DEBUG, "check if all files in %s belongs to %s\n", + dir, dbpkg->name); + resolved_conflict = dir_belongsto_pkg(filestr, dbpkg); + } + free(dir); + } + if(!resolved_conflict) { _alpm_log(PM_LOG_DEBUG, "file found in conflict: %s\n", path); conflicts = add_fileconflict(conflicts, PM_FILECONFLICT_FILESYSTEM, diff --git a/pactest/tests/fileconflict004.py b/pactest/tests/fileconflict004.py index 2396ced..a5347cc 100644 --- a/pactest/tests/fileconflict004.py +++ b/pactest/tests/fileconflict004.py @@ -17,5 +17,3 @@ self.addrule("PKG_EXIST=pkg1") self.addrule("PKG_VERSION=pkg1|2.0-1") self.addrule("FILE_TYPE=test|link") - -self.expectfailure = True diff --git a/pactest/tests/fileconflict005.py b/pactest/tests/fileconflict005.py index b9c0fa9..5c554af 100644 --- a/pactest/tests/fileconflict005.py +++ b/pactest/tests/fileconflict005.py @@ -20,4 +20,3 @@ self.addrule("PACMAN_RETCODE=1") self.addrule("PKG_EXIST=pkg1") self.addrule("PKG_VERSION=pkg1|1.0-1") - diff --git a/pactest/tests/fileconflict006.py b/pactest/tests/fileconflict006.py new file mode 100644 index 0000000..84afff2 --- /dev/null +++ b/pactest/tests/fileconflict006.py @@ -0,0 +1,24 @@ +self.description = "dir->symlink change during package upgrade (conflict)" + +p1 = pmpkg("pkg1", "1.0-1") +p1.files = ["test/", + "test/file1", + "test/dir/file1", + "test/dir/file2"] +self.addpkg2db("local", p1) + +p2 = pmpkg("pkg2") +p2.files = ["test/dir/file3"] +self.addpkg2db("local", p2) + +p3 = pmpkg("pkg1", "2.0-1") +p3.files = ["test2/", + "test2/file3", + "test -> test2"] +self.addpkg2db("sync", p3) + +self.args = "-S pkg1" + +self.addrule("PACMAN_RETCODE=1") +self.addrule("PKG_EXIST=pkg1") +self.addrule("PKG_VERSION=pkg1|1.0-1")
On Sun, Jul 19, 2009 at 7:12 AM, Xavier Chantryshiningxc@gmail.com wrote:
When one package wants to replace a directory by a file, we check that all files in that directory were owned by that package.
Additionally pacman can be more verbose when the extraction of the symlink (or file) fails. The patch to add.c looks more complex than it is, I just moved and reindented code to handle cases 10 and 11 together.
Signed-off-by: Xavier Chantry shiningxc@gmail.com
I like this, thanks!
-Dan
pacman-dev@lists.archlinux.org