classification
Title: packaging: fix database to stop skipping uninstall tests on win32
Type: Stage: resolved
Components: Distutils2 Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: eric.araujo Nosy List: alexis, eric.araujo, python-dev, tarek, thomas.holmes
Priority: high Keywords: patch

Created on 2011-07-06 04:29 by thomas.holmes, last changed 2011-07-08 15:23 by eric.araujo. This issue is now closed.

Files
File name Uploaded Description Edit
dcd66ae649b1-2.diff thomas.holmes, 2011-07-06 05:01 review
6e15ba060803.diff thomas.holmes, 2011-07-08 01:09 review
Repositories containing patches
https://bitbucket.org/thomas.holmes/cpython#work-packaging-uninstall
Messages (8)
msg139922 - (view) Author: Thomas Holmes (thomas.holmes) Date: 2011-07-06 04:34
Functions test_uninstall.test_uninstall and test_uninstall.test_remove_issue were disabled for win32. Upon enabling them they generated failures.

I have worked up a patch that refactors a packaging.Distribution function _get_records from using a generator to returning a list. The generator function was holding open the RECORD file that it is trying to delete, resulting in failure.

I've tried to follow the protocol for a distutils2 patch as shown here (http://wiki.python.org/moin/Distutils/Contributing) so hopefully I've got this remote repository pointing correct.

> packaging.tests.test_uninstall -v
test_remove_issue (__main__.UninstallTestCase) ... ERROR
FAIL
test_uninstall (__main__.UninstallTestCase) ... ERROR
FAIL
test_uninstall_unknow_distribution (__main__.UninstallTestCase) ... ok

======================================================================
ERROR: test_remove_issue (__main__.UninstallTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\python\dev\cpython\lib\packaging\tests\test_uninstall.py", line 48, in tearDown
    super(UninstallTestCase, self).tearDown()
  File "D:\python\dev\cpython\lib\packaging\tests\support.py", line 134, in tearDown
    shutil.rmtree(self._basetempdir)
  File "D:\python\dev\cpython\lib\shutil.py", line 279, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "D:\python\dev\cpython\lib\shutil.py", line 279, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "D:\python\dev\cpython\lib\shutil.py", line 279, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "D:\python\dev\cpython\lib\shutil.py", line 279, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "D:\python\dev\cpython\lib\shutil.py", line 284, in rmtree
    onerror(os.remove, fullname, sys.exc_info())
  File "D:\python\dev\cpython\lib\shutil.py", line 282, in rmtree
    os.remove(fullname)
WindowsError: [Error 32] The process cannot access the file because it is being used by another process: 'd:\\temp\\tmpy
6drm5\\tmp6htvi1\\Lib\\site-packages\\Meh-0.1.dist-info\\RECORD'

======================================================================
ERROR: test_uninstall (__main__.UninstallTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\python\dev\cpython\lib\packaging\tests\test_uninstall.py", line 48, in tearDown
    super(UninstallTestCase, self).tearDown()
  File "D:\python\dev\cpython\lib\packaging\tests\support.py", line 134, in tearDown
    shutil.rmtree(self._basetempdir)
  File "D:\python\dev\cpython\lib\shutil.py", line 279, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "D:\python\dev\cpython\lib\shutil.py", line 279, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "D:\python\dev\cpython\lib\shutil.py", line 279, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "D:\python\dev\cpython\lib\shutil.py", line 279, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "D:\python\dev\cpython\lib\shutil.py", line 284, in rmtree
    onerror(os.remove, fullname, sys.exc_info())
  File "D:\python\dev\cpython\lib\shutil.py", line 282, in rmtree
    os.remove(fullname)
WindowsError: [Error 32] The process cannot access the file because it is being used by another process: 'd:\\temp\\tmp4
23fq4\\tmpp2v6uq\\Lib\\site-packages\\Foo-0.1.dist-info\\RECORD'

======================================================================
FAIL: test_remove_issue (__main__.UninstallTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\python\dev\cpython\lib\packaging\tests\test_uninstall.py", line 124, in test_remove_issue
    self.assertTrue(remove('Meh', paths=[install_lib]))
AssertionError: False is not true

======================================================================
FAIL: test_uninstall (__main__.UninstallTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\python\dev\cpython\lib\packaging\tests\test_uninstall.py", line 102, in test_uninstall
    self.assertTrue(remove('Foo', paths=[install_lib]))
AssertionError: False is not true

----------------------------------------------------------------------
Ran 3 tests in 0.120s

FAILED (failures=2, errors=2)
[145911 refs]

D:\python\dev\cpython\PCbuild>
msg139983 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-07-07 17:39
Thanks for that!  I agree that it’s a fake optimization to work with generators instead of lists when the list is small or when we depend on other resources like file handles.  There are a few methods we could change in the database module.

The patch looks good, except that I wouldn’t move code to a nested function, but just change the “yield spam” line to “results.append(spam)”.

In our message, is the test log showing failure something you got before making changes or after?  IOW, does the patch fixes the bug?
msg139989 - (view) Author: Thomas Holmes (thomas.holmes) Date: 2011-07-07 18:05
The output in my initial message is the output of the tests with them enabled but pre database.py patch. Once the patch is applied all packaging tests that run on my system pass.

I was 50/50 on whether or not to use the internal function and I just sort of ended up with it. I had started out writing some more complex map/list comprehension stuff and the function made sense at the time.

Regarding the use of yield, at least one of the other functions that uses the _get_records function yields the result of _get_records. I figured those could be altered to just return the list directly but I decided to opt for the path of least modification prior to getting input.

If you think it is a good idea I will modify the patch to remove the unnecessary yields and insert the nested function code directly in _get_records instead.
msg139990 - (view) Author: Thomas Holmes (thomas.holmes) Date: 2011-07-07 18:06
Oh and thank you very much for your input. My apologies for the initial 9 e-mail spam when I created the issue, I bumbled the remote HG repository patch generation :)
msg140011 - (view) Author: Thomas Holmes (thomas.holmes) Date: 2011-07-08 01:09
I have made the change you suggested, creating a new list and simply amending to minimize the diff. This new patch has been attached.

I looked through the rest of database.py and did not see any other generators that appeared to erroneously hold a file handle open. If you are able to identify one that actually is a problem I would be happy to change it.

In addition there is one generator function (list_distinfo_files) which feeds off of the generator I have changed but I didn't see a need to alter them as they work fine as generators.
msg140020 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-07-08 11:19
Excellent!  I will apply this.

> I looked through the rest of database.py and did not see any other
> generators that appeared to erroneously hold a file handle open.
Me neither.  Thanks for checking.

> In addition there is one generator function (list_distinfo_files)
> which feeds off of the generator I have changed but I didn't see a
> need to alter them as they work fine as generators.
I think using a generator can save some memory, if the underlying list is huge, but the important thing here is that all functions with a similar name (list_*) behave in a consistent way, i.e. returning lists or generators but not a mix.
msg140032 - (view) Author: Roundup Robot (python-dev) Date: 2011-07-08 15:22
New changeset 2b9a0a091566 by Éric Araujo in branch 'default':
Close file handles in a timely manner in packaging.database (#12504).
http://hg.python.org/cpython/rev/2b9a0a091566
msg140033 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-07-08 15:23
Edited a bit and committed.  Thanks again!
History
Date User Action Args
2011-09-19 16:09:15eric.araujolinkissue12395 superseder
2011-07-08 15:23:58eric.araujosetstatus: open -> closed
title: packaging: fix uninstall and stop skipping its tests -> packaging: fix database to stop skipping uninstall tests on win32
messages: + msg140033

resolution: fixed
stage: patch review -> resolved
2011-07-08 15:22:40python-devsetnosy: + python-dev
messages: + msg140032
2011-07-08 11:19:15eric.araujosetpriority: normal -> high

messages: + msg140020
2011-07-08 01:09:39thomas.holmessetfiles: + 6e15ba060803.diff
2011-07-08 01:09:09thomas.holmessetmessages: + msg140011
2011-07-07 18:06:15thomas.holmessetmessages: + msg139990
2011-07-07 18:05:14thomas.holmessetmessages: + msg139989
2011-07-07 17:39:30eric.araujosetassignee: tarek -> eric.araujo
title: Uninstall has disabled windows tests -> packaging: fix uninstall and stop skipping its tests
messages: + msg139983
stage: patch review
2011-07-06 05:01:33thomas.holmessetfiles: + dcd66ae649b1-2.diff
2011-07-06 04:53:42thomas.holmessetfiles: - dcd66ae649b1.diff
2011-07-06 04:52:47thomas.holmessetfiles: + dcd66ae649b1.diff
2011-07-06 04:52:23thomas.holmessetfiles: - dd470b122f32.diff
2011-07-06 04:43:29thomas.holmessetfiles: + dd470b122f32.diff
2011-07-06 04:35:35thomas.holmessetfiles: - f333cd40cd56.diff
2011-07-06 04:35:17thomas.holmessetfiles: + f333cd40cd56.diff
keywords: + patch
2011-07-06 04:34:41thomas.holmessethgrepos: + hgrepo37
messages: + msg139922
2011-07-06 04:29:39thomas.holmescreate