From 9c4f44f70acadd70fcdb0a0d5d66cefbc9da20bf Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Tue, 15 Mar 2011 14:56:39 -0400 Subject: [PATCH 1/5] Fix issue #11432. if the stdin pipe is the same file descriptor as either stdout or stderr in the _posixsubprocess C extension module it would unintentionally close the fds and raise an error. --- Modules/_posixsubprocess.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 0f85da97cea..bf10cbb43e1 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -99,10 +99,10 @@ static void child_exec(char *const exec_array[], if (p2cread > 2) { POSIX_CALL(close(p2cread)); } - if (c2pwrite > 2) { + if (c2pwrite > 2 && c2pwrite != p2cread) { POSIX_CALL(close(c2pwrite)); } - if (errwrite != c2pwrite && errwrite > 2) { + if (errwrite != c2pwrite && errwrite != p2cread && errwrite > 2) { POSIX_CALL(close(errwrite)); } From e14e9c2218dc449a374a31a5bc4f9e1a82ef61fa Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Tue, 15 Mar 2011 14:55:17 -0400 Subject: [PATCH 2/5] Add unittests demonstrating issue #11432. --- Lib/test/test_subprocess.py | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 73f44ad93af..46e50c350bf 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -3,6 +3,7 @@ from test import support import subprocess import sys import signal +import io import os import errno import tempfile @@ -1186,6 +1187,24 @@ class POSIXProcessTestCase(BaseTestCase): close_fds=False, pass_fds=(fd, ))) self.assertIn('overriding close_fds', str(context.warning)) + def test_stdout_stdin_are_single_inout_fd(self): + with io.open(os.devnull, "r+") as inout: + p = subprocess.Popen([sys.executable, "-c", "import sys; sys.exit(0)"], + stdout=inout, stdin=inout) + p.wait() + + def test_stdout_stderr_are_single_inout_fd(self): + with io.open(os.devnull, "r+") as inout: + p = subprocess.Popen([sys.executable, "-c", "import sys; sys.exit(0)"], + stdout=inout, stderr=inout) + p.wait() + + def test_stderr_stdin_are_single_inout_fd(self): + with io.open(os.devnull, "r+") as inout: + p = subprocess.Popen([sys.executable, "-c", "import sys; sys.exit(0)"], + stderr=inout, stdin=inout) + p.wait() + def test_wait_when_sigchild_ignored(self): # NOTE: sigchild_ignore.py may not be an effective test on all OSes. sigchild_ignore = support.findfile("sigchild_ignore.py", @@ -1458,19 +1477,6 @@ class ContextManagerTests(ProcessTestCase): raise c.exception -def test_main(): - unit_tests = (ProcessTestCase, - POSIXProcessTestCase, - Win32ProcessTestCase, - ProcessTestCasePOSIXPurePython, - CommandTests, - ProcessTestCaseNoPoll, - HelperFunctionTests, - CommandsWithSpaces, - ContextManagerTests) - - support.run_unittest(*unit_tests) - support.reap_children() - if __name__ == "__main__": - test_main() + unittest.main() + support.reap_children() From 8f7724f9a4ec57d689a436a064e2e047b2ad0d97 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Tue, 15 Mar 2011 15:24:43 -0400 Subject: [PATCH 3/5] merge d71476b9a55d from tip, use start_new_session instead of os.setsid. --- Lib/webbrowser.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/Lib/webbrowser.py b/Lib/webbrowser.py index e369acb1df6..415f12ac5f3 100644 --- a/Lib/webbrowser.py +++ b/Lib/webbrowser.py @@ -228,15 +228,9 @@ class UnixBrowser(BaseBrowser): else: # for TTY browsers, we need stdin/out inout = None - # if possible, put browser in separate process group, so - # keyboard interrupts don't affect browser as well as Python - setsid = getattr(os, 'setsid', None) - if not setsid: - setsid = getattr(os, 'setpgrp', None) - p = subprocess.Popen(cmdline, close_fds=True, stdin=inout, stdout=(self.redirect_stdout and inout or None), - stderr=inout, preexec_fn=setsid) + stderr=inout, start_new_session=True) if remote: # wait five secons. If the subprocess is not finished, the # remote invocation has (hopefully) started a new instance. From 961e0e85c007677ba4406382d6951a39487d440c Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Tue, 15 Mar 2011 15:43:39 -0400 Subject: [PATCH 4/5] revert the test_main() change from 08daf3ef6509 so that regrtest continues to run this properly. --- Lib/test/test_subprocess.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 46e50c350bf..3cc387b6a3d 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1477,6 +1477,19 @@ class ContextManagerTests(ProcessTestCase): raise c.exception +def test_main(): + unit_tests = (ProcessTestCase, + POSIXProcessTestCase, + Win32ProcessTestCase, + ProcessTestCasePOSIXPurePython, + CommandTests, + ProcessTestCaseNoPoll, + HelperFunctionTests, + CommandsWithSpaces, + ContextManagerTests) + + support.run_unittest(*unit_tests) + support.reap_children() + if __name__ == "__main__": unittest.main() - support.reap_children() From 2c50a09ac47e701768a4e20f8a03d17263e8db8f Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Tue, 15 Mar 2011 21:02:59 +0100 Subject: [PATCH 5/5] On behalf of Tarek: Issue #11501: disutils.archive_utils.make_zipfile no longer fails if zlib is not installed. Instead, the zipfile.ZIP_STORED compression is used to create the ZipFile. Patch by Natalia B. Bidart. --- Lib/distutils/archive_util.py | 19 +++++++---- Lib/distutils/tests/test_archive_util.py | 42 +++++++++++++++++++++--- Lib/distutils/tests/test_bdist_dumb.py | 8 +++++ Lib/distutils/tests/test_sdist.py | 15 ++++++++- Lib/test/support.py | 33 +++++++++++++++++++ Misc/ACKS | 1 + Misc/NEWS | 4 +++ 7 files changed, 110 insertions(+), 12 deletions(-) diff --git a/Lib/distutils/archive_util.py b/Lib/distutils/archive_util.py index 6dd0445dbe6..c06eba351d8 100644 --- a/Lib/distutils/archive_util.py +++ b/Lib/distutils/archive_util.py @@ -9,6 +9,12 @@ import os from warnings import warn import sys +try: + import zipfile +except ImportError: + zipfile = None + + from distutils.errors import DistutilsExecError from distutils.spawn import spawn from distutils.dir_util import mkpath @@ -74,11 +80,6 @@ def make_zipfile(base_name, base_dir, verbose=0, dry_run=0): available, raises DistutilsExecError. Returns the name of the output zip file. """ - try: - import zipfile - except ImportError: - zipfile = None - zip_filename = base_name + ".zip" mkpath(os.path.dirname(zip_filename), dry_run=dry_run) @@ -105,8 +106,12 @@ def make_zipfile(base_name, base_dir, verbose=0, dry_run=0): zip_filename, base_dir) if not dry_run: - zip = zipfile.ZipFile(zip_filename, "w", - compression=zipfile.ZIP_DEFLATED) + try: + zip = zipfile.ZipFile(zip_filename, "w", + compression=zipfile.ZIP_DEFLATED) + except RuntimeError: + zip = zipfile.ZipFile(zip_filename, "w", + compression=zipfile.ZIP_STORED) for dirpath, dirnames, filenames in os.walk(base_dir): for name in filenames: diff --git a/Lib/distutils/tests/test_archive_util.py b/Lib/distutils/tests/test_archive_util.py index aff426521d9..f9698495903 100644 --- a/Lib/distutils/tests/test_archive_util.py +++ b/Lib/distutils/tests/test_archive_util.py @@ -7,12 +7,13 @@ import tarfile from os.path import splitdrive import warnings +from distutils import archive_util from distutils.archive_util import (check_archive_formats, make_tarball, make_zipfile, make_archive, ARCHIVE_FORMATS) from distutils.spawn import find_executable, spawn from distutils.tests import support -from test.support import check_warnings, run_unittest +from test.support import check_warnings, run_unittest, patch try: import zipfile @@ -20,10 +21,18 @@ try: except ImportError: ZIP_SUPPORT = find_executable('zip') +try: + import zlib + ZLIB_SUPPORT = True +except ImportError: + ZLIB_SUPPORT = False + + class ArchiveUtilTestCase(support.TempdirManager, support.LoggingSilencer, unittest.TestCase): + @unittest.skipUnless(ZLIB_SUPPORT, 'Need zlib support to run') def test_make_tarball(self): # creating something to tar tmpdir = self.mkdtemp() @@ -84,8 +93,9 @@ class ArchiveUtilTestCase(support.TempdirManager, base_name = os.path.join(tmpdir2, 'archive') return tmpdir, tmpdir2, base_name - @unittest.skipUnless(find_executable('tar') and find_executable('gzip'), - 'Need the tar command to run') + @unittest.skipUnless(find_executable('tar') and find_executable('gzip') + and ZLIB_SUPPORT, + 'Need the tar, gzip and zlib command to run') def test_tarfile_vs_tar(self): tmpdir, tmpdir2, base_name = self._create_files() old_dir = os.getcwd() @@ -169,7 +179,8 @@ class ArchiveUtilTestCase(support.TempdirManager, self.assertTrue(not os.path.exists(tarball)) self.assertEqual(len(w.warnings), 1) - @unittest.skipUnless(ZIP_SUPPORT, 'Need zip support to run') + @unittest.skipUnless(ZIP_SUPPORT and ZLIB_SUPPORT, + 'Need zip and zlib support to run') def test_make_zipfile(self): # creating something to tar tmpdir = self.mkdtemp() @@ -182,6 +193,29 @@ class ArchiveUtilTestCase(support.TempdirManager, # check if the compressed tarball was created tarball = base_name + '.zip' + self.assertTrue(os.path.exists(tarball)) + + @unittest.skipUnless(ZIP_SUPPORT, 'Need zip support to run') + def test_make_zipfile_no_zlib(self): + patch(self, archive_util.zipfile, 'zlib', None) # force zlib ImportError + + called = [] + zipfile_class = zipfile.ZipFile + def fake_zipfile(*a, **kw): + if kw.get('compression', None) == zipfile.ZIP_STORED: + called.append((a, kw)) + return zipfile_class(*a, **kw) + + patch(self, archive_util.zipfile, 'ZipFile', fake_zipfile) + + # create something to tar and compress + tmpdir, tmpdir2, base_name = self._create_files() + make_zipfile(base_name, tmpdir) + + tarball = base_name + '.zip' + self.assertEqual(called, + [((tarball, "w"), {'compression': zipfile.ZIP_STORED})]) + self.assertTrue(os.path.exists(tarball)) def test_check_archive_formats(self): self.assertEqual(check_archive_formats(['gztar', 'xxx', 'zip']), diff --git a/Lib/distutils/tests/test_bdist_dumb.py b/Lib/distutils/tests/test_bdist_dumb.py index cc37fef8b01..55ba58d14fa 100644 --- a/Lib/distutils/tests/test_bdist_dumb.py +++ b/Lib/distutils/tests/test_bdist_dumb.py @@ -18,6 +18,13 @@ setup(name='foo', version='0.1', py_modules=['foo'], """ +try: + import zlib + ZLIB_SUPPORT = True +except ImportError: + ZLIB_SUPPORT = False + + class BuildDumbTestCase(support.TempdirManager, support.LoggingSilencer, support.EnvironGuard, @@ -34,6 +41,7 @@ class BuildDumbTestCase(support.TempdirManager, sys.argv[:] = self.old_sys_argv[1] super(BuildDumbTestCase, self).tearDown() + @unittest.skipUnless(ZLIB_SUPPORT, 'Need zlib support to run') def test_simple_built(self): # let's create a simple package diff --git a/Lib/distutils/tests/test_sdist.py b/Lib/distutils/tests/test_sdist.py index 655e50dcfc2..eaf39a45bae 100644 --- a/Lib/distutils/tests/test_sdist.py +++ b/Lib/distutils/tests/test_sdist.py @@ -40,6 +40,13 @@ somecode%(sep)sdoc.dat somecode%(sep)sdoc.txt """ +try: + import zlib + ZLIB_SUPPORT = True +except ImportError: + ZLIB_SUPPORT = False + + class SDistTestCase(PyPIRCCommandTestCase): def setUp(self): @@ -78,6 +85,7 @@ class SDistTestCase(PyPIRCCommandTestCase): cmd.warn = _warn return dist, cmd + @unittest.skipUnless(ZLIB_SUPPORT, 'Need zlib support to run') def test_prune_file_list(self): # this test creates a package with some vcs dirs in it # and launch sdist to make sure they get pruned @@ -119,6 +127,7 @@ class SDistTestCase(PyPIRCCommandTestCase): # making sure everything has been pruned correctly self.assertEqual(len(content), 4) + @unittest.skipUnless(ZLIB_SUPPORT, 'Need zlib support to run') def test_make_distribution(self): # check if tar and gzip are installed @@ -153,6 +162,7 @@ class SDistTestCase(PyPIRCCommandTestCase): result.sort() self.assertEqual(result, ['fake-1.0.tar', 'fake-1.0.tar.gz']) + @unittest.skipUnless(ZLIB_SUPPORT, 'Need zlib support to run') def test_add_defaults(self): # http://bugs.python.org/issue2279 @@ -218,6 +228,7 @@ class SDistTestCase(PyPIRCCommandTestCase): finally: f.close() + @unittest.skipUnless(ZLIB_SUPPORT, 'Need zlib support to run') def test_metadata_check_option(self): # testing the `medata-check` option dist, cmd = self.get_cmd(metadata={}) @@ -277,7 +288,7 @@ class SDistTestCase(PyPIRCCommandTestCase): cmd.formats = 'supazipa' self.assertRaises(DistutilsOptionError, cmd.finalize_options) - + @unittest.skipUnless(ZLIB_SUPPORT, 'Need zlib support to run') def test_get_file_list(self): # make sure MANIFEST is recalculated dist, cmd = self.get_cmd() @@ -318,6 +329,7 @@ class SDistTestCase(PyPIRCCommandTestCase): self.assertEqual(len(manifest2), 6) self.assertIn('doc2.txt', manifest2[-1]) + @unittest.skipUnless(ZLIB_SUPPORT, 'Need zlib support to run') def test_manifest_marker(self): # check that autogenerated MANIFESTs have a marker dist, cmd = self.get_cmd() @@ -334,6 +346,7 @@ class SDistTestCase(PyPIRCCommandTestCase): self.assertEqual(manifest[0], '# file GENERATED by distutils, do NOT edit') + @unittest.skipUnless(ZLIB_SUPPORT, 'Need zlib support to run') def test_manual_manifest(self): # check that a MANIFEST without a marker is left alone dist, cmd = self.get_cmd() diff --git a/Lib/test/support.py b/Lib/test/support.py index 9fb3ee00c10..bef7161e9a1 100644 --- a/Lib/test/support.py +++ b/Lib/test/support.py @@ -1096,3 +1096,36 @@ def strip_python_stderr(stderr): """ stderr = re.sub(br"\[\d+ refs\]\r?\n?$", b"", stderr).strip() return stderr + +def patch(test_instance, object_to_patch, attr_name, new_value): + """Override 'object_to_patch'.'attr_name' with 'new_value'. + + Also, add a cleanup procedure to 'test_instance' to restore + 'object_to_patch' value for 'attr_name'. + The 'attr_name' should be a valid attribute for 'object_to_patch'. + + """ + # check that 'attr_name' is a real attribute for 'object_to_patch' + # will raise AttributeError if it does not exist + getattr(object_to_patch, attr_name) + + # keep a copy of the old value + attr_is_local = False + try: + old_value = object_to_patch.__dict__[attr_name] + except (AttributeError, KeyError): + old_value = getattr(object_to_patch, attr_name, None) + else: + attr_is_local = True + + # restore the value when the test is done + def cleanup(): + if attr_is_local: + setattr(object_to_patch, attr_name, old_value) + else: + delattr(object_to_patch, attr_name) + + test_instance.addCleanup(cleanup) + + # actually override the attribute + setattr(object_to_patch, attr_name, new_value) diff --git a/Misc/ACKS b/Misc/ACKS index fe80c8bbdfe..c7fd2ff9a03 100644 --- a/Misc/ACKS +++ b/Misc/ACKS @@ -72,6 +72,7 @@ Eric Beser Steven Bethard Stephen Bevan Ron Bickers +Natalia B. Bidart David Binger Dominic Binks Philippe Biondi diff --git a/Misc/NEWS b/Misc/NEWS index d4ce939119f..c4627fac66a 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -40,6 +40,10 @@ Core and Builtins Library ------- +- Issue #11501: disutils.archive_utils.make_zipfile no longer fails if zlib is + not installed. Instead, the zipfile.ZIP_STORED compression is used to create + the ZipFile. Patch by Natalia B. Bidart. + - Issue #11491: dbm.error is no longer raised when dbm.open is called with the "n" as the flag argument and the file exists. The behavior matches the documentation and general logic.