Re: [Python-Dev] [Python-checkins] r66863 - python/trunk/Modules/posixmodule.c
[switching to python-dev] Georg Brandl wrote:
Martin v. Löwis schrieb:
Raymond Hettinger wrote:
Merges should be handled by the original committer. Respectfully disagree. Some people here having been taking responsibility for keeping the branches in-sync Which people specifically?
Specifically, Christian, Benjamin and myself have done larger merges to the 3k branch in the past, and if svnmerge is used, I suspect will do the same for 2.6.
That's different, though. Does any of you has actually taken *responsibility* to do so, either unlimited, or with some limitation? (e.g. for a year, or until 2.7 is released, or for all changes but bsddb and Windows). I would be (somewhat) happy to hear that, but I wouldn't really expect it - we are all volunteers, and we typically consider taking responsibility (e.g. as a release manager) very carefully. Please don't get me wrong: I very much appreciate that you volunteer, but I don't want to discharge any committer from merging on the assumption that someone has formally taken responsibility. I would be skeptical relying on such a commitment, knowing that RL can get in the way too easily. E.g. Christian disappeared for some time, and I completely sympathize with that - but it also tells me that I can't trust on somebody doing something unless that someone has explicitly said that he will do that, hoping that he will tell me when the commitment is no longer valid (the same happened, e.g., in the Python job board, and happens all the time in other projects - it took me about a year before I stepped back as the PyXML maintainer). I can *also* sympathize with committers that say "we don't want to backport, because we either don't have the time, or the expertise (say, to install and run svnmerge on Windows)". I just accept that not all patches that deserve backporting actually do get backported (just as not all patches that deserve acceptance do get accepted, in the first place). Regards, Martin
Hello, Concerning the management of this particular change / development, I see three additional issues: - First, I think that the answer given here: http://mail.python.org/pipermail/python-checkins/2008-October/074659.html does not match the question. It seems to me that Skip was asking whether the "memory leak" impacted the 2.6 branch, and the answer should have been "No": the change that introduced the memory leak had just been committed 10 minutes before. - Because of this misunderstanding, the changes to this GetCurrentDirectoryW were backported to the release2.6 branch, despite the fact that it's not a regression from a previous version, the NEWS entry explicitly expresses doubts about the correction (which I happen to share), there is no unit test and no item in the issue tracker. - The backport to release26-maint http://svn.python.org/view?rev=66865&view=rev also merged other changes (new unrelated unit tests). IMO unrelated changes should be committed separately: different commit messages help to understand the motivation of each backport. I'm not blaming anyone; my feelings are certainly biased by the somewhat strict procedures that we recently followed when the trunk was in "release candidate" mode (submit a patch, ask for a review, add the reviewer's name in the commit message). I think that some of these rules should still apply to the maintenance branches. On the other hand, I am all for a trunk branch with few restrictions: it's here to share code with others and experiment with the next python release. The need for stability is much lower. -- Amaury Forgeot d'Arc
It seems to me that Skip was asking whether the "memory leak" impacted the 2.6 branch, and the answer should have been "No": the change that introduced the memory leak had just been committed 10 minutes before.
You are probably right (although it's not quite clear from Skip's question).
- Because of this misunderstanding, the changes to this GetCurrentDirectoryW were backported to the release2.6 branch, despite the fact that it's not a regression from a previous version, the NEWS entry explicitly expresses doubts about the correction (which I happen to share), there is no unit test and no item in the issue tracker.
I think it is fine that this fix was backported (assuming, without review, that the fix is actually correct). It is a bugfix, and it shouldn't realistically break existing applications. IOW, PEP 6 was followed (except that there is no Patch Czar).
- The backport to release26-maint http://svn.python.org/view?rev=66865&view=rev also merged other changes (new unrelated unit tests). IMO unrelated changes should be committed separately: different commit messages help to understand the motivation of each backport.
Yes, that is unfortunate. I'm skeptical that new tests actually need backporting at all. Python doesn't really get better by new tests being added to an old branch. Near-term, it might get worse because the new tests might cause false positives, making users worried for no reason. Regards, Martin
On 11:12 pm, martin@v.loewis.de wrote:
- The backport to release26-maint http://svn.python.org/view?rev=66865&view=rev also merged other changes (new unrelated unit tests).
IMO unrelated changes should be committed separately: different commit messages help to understand the motivation of each backport.
This I agree with completely, but...
I'm skeptical that new tests actually need backporting at all. Python doesn't really get better by new tests being added to an old branch.
Presumably if the new tests cover functionality that somebody cares about, it would be valuable to make sure that maintenance bugfixes don't break this functionality in maintenance branches either? In fact, I would think that maintenance branches would want to be much _more_ paranoid about making sure that changes don't break anything. (Again: not specific to these unrelated tests that are getting merged, it sounds like accidentally - it just seems strange, as a general principle or policy, to say that maintenance branches shouldn't have new tests added to them.)
Presumably if the new tests cover functionality that somebody cares about, it would be valuable to make sure that maintenance bugfixes don't break this functionality in maintenance branches either?
Yes. At the same time, there is also a risk that the new tests just fail because they are not correctly formulated. Users don't report that against the trunk, because they are not running the trunk. Instead, they download the next maintenance release, run the tests, see that it fails, get frightened, and return to the prior release (which didn't have any failing tests). There is a certain prevention already that later maintenance fixes don't break the earlier ones: those fixes typically get checked into the trunk also, where the tests do exist. So the committer would find out even before the patch gets to the maintenance branch. All that said, I don't want to state a policy about whether new tests should or should not get added to maintenance branches. I'd leave this up to the committers, and just wanted to caution about this risk. Regards, Martin
"Martin v. Löwis" wrote: [...]
There is a certain prevention already that later maintenance fixes don't break the earlier ones: those fixes typically get checked into the trunk also, where the tests do exist. So the committer would find out even before the patch gets to the maintenance branch.
By this logic, the maintenance branch would need no tests at all, because they are all in trunk already! -Andrew.
Andrew Bennetts wrote:
"Martin v. Löwis" wrote: [...]
There is a certain prevention already that later maintenance fixes don't break the earlier ones: those fixes typically get checked into the trunk also, where the tests do exist. So the committer would find out even before the patch gets to the maintenance branch.
By this logic, the maintenance branch would need no tests at all, because they are all in trunk already!
No, this is not the logic. The tests in the maintenance branch have gone through alpha and beta releases, so end users have seen them already (at least, some of them). Of course, it might still be possible that there is an incorrect test in the released version; those can get fixed in later maintenance releases. So 2.6.0 will contain a lot of tests that have never been tested in a wide variety of systems. Some are incorrect, and get fixed in 2.6.1, and stay fixed afterwards. This is completely different from somebody introducing a new test in 2.6.4. It means that there are more failures in a maintenance release, not less as in the first case. Of course, it might be that a developer deliberately wants to see a certain test be run in the field, because there is a uncertainty whether the fix actually works. However, it is questionable whether such a fix would belong in the maintenance release. Regards, Martin
On Fri, Oct 10, 2008 at 08:44:38AM +0200, "Martin v. Löwis" wrote:
So 2.6.0 will contain a lot of tests that have never been tested in a wide variety of systems. Some are incorrect, and get fixed in 2.6.1, and stay fixed afterwards. This is completely different from somebody introducing a new test in 2.6.4. It means that there are more failures in a maintenance release, not less as in the first case.
Indeed. Looking at the commit logs, I had to split out all the test-only commits into a separate list, and tests often took several attempts to get working on all platforms. I think backporting should be prioritized into 1) bug fixes, 2) documentation improvements, 3) tests. For 2.5.3, we should just ignore 2) and 3); documentation patches will require rewriting from reST into LaTeX, which is too much effort for the return, and tests are even less useful to the end users, being primarily useful for Python's developers. (2.5.3 reminder: there are lists of commits in sandbox/py2.5.3 to be considered. I've seen no reactions on python-dev or modifications to those files, so I don't think anyone else is looking at them. Is everyone waiting for the weekend, maybe?) --amk
(2.5.3 reminder: there are lists of commits in sandbox/py2.5.3 to be considered. I've seen no reactions on python-dev or modifications to those files, so I don't think anyone else is looking at them. Is everyone waiting for the weekend, maybe?)
I may have said that before: I don't have any plans to look through change lists myself. If people want certain changes considered, they will tell us; if nobody is interested in a certain backport, there is no need to backport. Regards, Martin
"Martin v. Löwis" writes:
I'm skeptical that new tests actually need backporting at all. Python doesn't really get better by new tests being added to an old branch. Near-term, it might get worse because the new tests might cause false positives, making users worried for no reason.
If they do fail, they're not "false" positives. If they're "false", then the test is broken, no? So find a way to label them as tests added ex-post, with the failures *not* being regressions but rather latent bugs newly detected, and (presumably) as "wont-fix".
From a QA point of view one would like to be able to assess how many latent bugs are making it through to end-of-life. The new tests will help in that.
If they do fail, they're not "false" positives. If they're "false", then the test is broken, no?
Correct. But they might well be broken, no?
So find a way to label them as tests added ex-post, with the failures *not* being regressions but rather latent bugs newly detected, and (presumably) as "wont-fix".
No such way exists, hence they can't be labeled that way. No such labeling can be added, since that would be a new feature. Regards, Martin
"Martin v. Löwis" writes:
If they do fail, they're not "false" positives. If they're "false", then the test is broken, no?
Correct. But they might well be broken, no?
I would hope some effort is made that they not be. If they generate a positive, I would expect that the contributor would try to fix that before committing, no? If they discover that it's "false", they fix or remove the test; otherwise they document it.
So find a way to label them as tests added ex-post, with the failures *not* being regressions but rather latent bugs newly detected, and (presumably) as "wont-fix".
No such way exists,
Add a documentation file called "README.expected-test-failures". AFAIK documentation is always acceptable, right? Whether that is an acceptable solution to the "latent bug" problem is a different question. I'd rather know that Python has unexpected behavior, and have a sample program (== test) to demonstrate it, than not. YMMV.
Correct. But they might well be broken, no?
I would hope some effort is made that they not be. If they generate a positive, I would expect that the contributor would try to fix that before committing, no? If they discover that it's "false", they fix or remove the test; otherwise they document it.
That assumes they know. We had recently a number of test cases that fixed security problems, and the tests would only run correctly on 32-bit systems. On 64-bit systems, they would consume all memory, and either bring the machine down, or complete eventually with a failure (because the expected out-of-memory situation did not arise). The author of the code was unaware of its dependency on the architecture, and the test would run fine on his machine. Likewise, we had test failures that only failed if a certain locale was not available on a system, or had locale definitions that were different from the ones on Linux. There is a lot of potential for tests to only fail on systems which we don't have access to.
Whether that is an acceptable solution to the "latent bug" problem is a different question. I'd rather know that Python has unexpected behavior, and have a sample program (== test) to demonstrate it, than not. YMMV.
And it does indeed for many people. We get a significant number of reports from people who find that the test suite fails, and then are unable to draw any conclusions from that other than Python is apparently broken, and that they therefore shouldn't use it - even if the test that fails is in a module that they likely never use. Regards, Martin
It seems to me that Skip was asking whether the "memory leak" impacted the 2.6 branch, and the answer should have been "No": the change that introduced the memory leak had just been committed 10 minutes before.
You are probably right (although it's not quite clear from Skip's question).
Umm, sorry for misunderstandings. I thought he indicated the set of two patches.
- Because of this misunderstanding, the changes to this GetCurrentDirectoryW were backported to the release2.6 branch, despite the fact that it's not a regression from a previous version, the NEWS entry explicitly expresses doubts about the correction (which I happen to share), there is no unit test and no item in the issue tracker.
I think it is fine that this fix was backported (assuming, without review, that the fix is actually correct).
It is a bugfix, and it shouldn't realistically break existing applications.
IOW, PEP 6 was followed (except that there is no Patch Czar).
Thanks, I'm a bit relaxed. :-)
- The backport to release26-maint http://svn.python.org/view?rev=66865&view=rev also merged other changes (new unrelated unit tests). IMO unrelated changes should be committed separately: different commit messages help to understand the motivation of each backport.
Yes, that is unfortunate.
I'm skeptical that new tests actually need backporting at all. Python doesn't really get better by new tests being added to an old branch. Near-term, it might get worse because the new tests might cause false positives, making users worried for no reason.
OK, I'll do separate commit for release26-maint even via svnmerge.py (I did same way as in py3k) But I'm bit confused. This is difficult problem for me, so I 'll commit to only trunk until some consensus will be established.
participants (7)
-
"Martin v. Löwis" -
A.M. Kuchling -
Amaury Forgeot d'Arc -
Andrew Bennetts -
glyph＠divmod.com -
Hirokazu Yamamoto -
Stephen J. Turnbull