Mailing List Archive: Python: Dev

Re: [Python-checkins] r66863 - python/trunk/Modules/posixmodule.c

Index | Next | Previous | View Threaded

martin at v

Oct 9, 2008, 4:12 PM

Post #1 of 12 (431 views)
Permalink

Re: [Python-checkins] r66863 - python/trunk/Modules/posixmodule.c

> 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
_______________________________________________
Python-Dev mailing list
Python-Dev[at]python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

glyph at divmod

Oct 9, 2008, 4:35 PM

Post #2 of 12 (418 views)
Permalink

Re: [Python-checkins] r66863 - python/trunk/Modules/posixmodule.c [In reply to]

On 11:12 pm, martin[at]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.)
_______________________________________________
Python-Dev mailing list
Python-Dev[at]python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

stephen at xemacs

Oct 9, 2008, 6:20 PM

Post #3 of 12 (417 views)
Permalink

Re: [Python-checkins] r66863 - python/trunk/Modules/posixmodule.c [In reply to]

"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.
_______________________________________________
Python-Dev mailing list
Python-Dev[at]python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

martin at v

Oct 9, 2008, 10:33 PM

Post #4 of 12 (412 views)
Permalink

Re: [Python-checkins] r66863 - python/trunk/Modules/posixmodule.c [In reply to]

> 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
_______________________________________________
Python-Dev mailing list
Python-Dev[at]python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

martin at v

Oct 9, 2008, 10:42 PM

Post #5 of 12 (412 views)
Permalink

Re: [Python-checkins] r66863 - python/trunk/Modules/posixmodule.c [In reply to]

> 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
_______________________________________________
Python-Dev mailing list
Python-Dev[at]python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

andrew-pythondev at puzzling

Oct 9, 2008, 11:29 PM

Post #6 of 12 (412 views)
Permalink

Re: [Python-checkins] r66863 - python/trunk/Modules/posixmodule.c [In reply to]

"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.

_______________________________________________
Python-Dev mailing list
Python-Dev[at]python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

martin at v

Oct 9, 2008, 11:44 PM

Post #7 of 12 (412 views)
Permalink

Re: [Python-checkins] r66863 - python/trunk/Modules/posixmodule.c [In reply to]

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
_______________________________________________
Python-Dev mailing list
Python-Dev[at]python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

stephen at xemacs

Oct 10, 2008, 12:02 AM

Post #8 of 12 (412 views)
Permalink

Re: [Python-checkins] r66863 - python/trunk/Modules/posixmodule.c [In reply to]

"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.

_______________________________________________
Python-Dev mailing list
Python-Dev[at]python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

martin at v

Oct 10, 2008, 12:32 AM

Post #9 of 12 (413 views)
Permalink

Re: [Python-checkins] r66863 - python/trunk/Modules/posixmodule.c [In reply to]

> > 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
_______________________________________________
Python-Dev mailing list
Python-Dev[at]python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

amk at amk

Oct 10, 2008, 8:44 AM

Post #10 of 12 (400 views)
Permalink

Re: [Python-checkins] r66863 - python/trunk/Modules/posixmodule.c [In reply to]

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
_______________________________________________
Python-Dev mailing list
Python-Dev[at]python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

ocean-city at m2

Oct 10, 2008, 3:07 PM

Post #11 of 12 (397 views)
Permalink

Re: [Python-checkins] r66863 - python/trunk/Modules/posixmodule.c [In reply to]

>> 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.
_______________________________________________
Python-Dev mailing list
Python-Dev[at]python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

martin at v

Oct 12, 2008, 11:15 AM

Post #12 of 12 (371 views)
Permalink

Re: [Python-checkins] r66863 - python/trunk/Modules/posixmodule.c [In reply to]

> (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
_______________________________________________
Python-Dev mailing list
Python-Dev[at]python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact lists@gossamer-threads.com