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>

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?

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.

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 :)

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.

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.

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

Edited a bit and committed.  Thanks again!