Skip to content

gh-94844: Add pathlib support to shutil archive management #94846

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Jul 20, 2022
Merged

gh-94844: Add pathlib support to shutil archive management #94846

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3912f2e
Add pathlib support to shutil.make_archive
arhadthedev Jul 14, 2022
828abde
Add pathlib support to shutil._make_tarball
arhadthedev Jul 14, 2022
49a2158
Extend TestArchives with pathlib cases
arhadthedev Jul 14, 2022
deebebc
Address a Barney's review
arhadthedev Jul 14, 2022
0e14249
Fix accidental passing of `self` into a non-method
arhadthedev Jul 14, 2022
26d736c
Move os.fspath call into a higher level
arhadthedev Jul 14, 2022
85bf9f1
Revert the extra set of tests and the replacement with f-strings
arhadthedev Jul 14, 2022
f099b32
Restore a newline missing in Github raw view
arhadthedev Jul 14, 2022
debd99b
Merge branch 'main' into fix-shutil-makearchive
arhadthedev Jul 16, 2022
ff94966
Address the Serhiy's review
arhadthedev Jul 16, 2022
bdaf3f9
Address the Barney's review
arhadthedev Jul 16, 2022
5a1c347
Add a forgotten comment from the review
arhadthedev Jul 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions Lib/shutil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -927,7 +927,7 @@ def _make_tarball(base_name, base_dir, compress="gzip", verbose=0, dry_run=0,
import tarfile # late import for breaking circular dependency

compress_ext = '.' + tar_compression if compress else ''
archive_name = base_name + '.tar' + compress_ext
archive_name = f'{base_name}.tar{compress_ext}'
archive_dir = os.path.dirname(archive_name)

if archive_dir and not os.path.exists(archive_dir):
Expand Down Expand Up @@ -975,7 +975,7 @@ def _make_zipfile(base_name, base_dir, verbose=0, dry_run=0,
"""
import zipfile # late import for breaking circular dependency

zip_filename = base_name + ".zip"
zip_filename = f'{base_name}.zip'
archive_dir = os.path.dirname(base_name)

if archive_dir and not os.path.exists(archive_dir):
Expand Down Expand Up @@ -1127,6 +1127,7 @@ def make_archive(base_name, format, root_dir=None, base_dir=None, verbose=0,
if not dry_run:
os.chdir(root_dir)

base_name = os.fspath(base_name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do it only if root_dir is not None, and only if support_root_dir is true (because os.path.abspath() already does the work).

It is an undocumented behavior and we do not want to introduce a new official feature in a bugfix release.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, amended.

try:
filename = func(base_name, base_dir, **kwargs)
finally:
Expand Down
58 changes: 37 additions & 21 deletions Lib/test/test_shutil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1327,9 +1327,9 @@ def test_copyfile_copy_dir(self):
self.assertRaises(err, shutil.copyfile, dir2, src_dir)


class TestArchives(BaseTest, unittest.TestCase):
class _ArchiveTests(BaseTest):

### shutil.make_archive
### shutil.make_archive with str, pathlib, and probably something else

@support.requires_zlib()
def test_make_tarball(self):
Expand All @@ -1345,7 +1345,7 @@ def test_make_tarball(self):

with os_helper.change_cwd(work_dir), no_chdir:
base_name = os.path.abspath(rel_base_name)
tarball = make_archive(rel_base_name, 'gztar', root_dir, '.')
tarball = self._make_archive(rel_base_name, 'gztar', root_dir, '.')

# check if the compressed tarball was created
self.assertEqual(tarball, base_name + '.tar.gz')
Expand All @@ -1358,7 +1358,7 @@ def test_make_tarball(self):

# trying an uncompressed one
with os_helper.change_cwd(work_dir), no_chdir:
tarball = make_archive(rel_base_name, 'tar', root_dir, '.')
tarball = self._make_archive(rel_base_name, 'tar', root_dir, '.')
self.assertEqual(tarball, base_name + '.tar')
self.assertTrue(os.path.isfile(tarball))
self.assertTrue(tarfile.is_tarfile(tarball))
Expand Down Expand Up @@ -1394,7 +1394,7 @@ def test_tarfile_vs_tar(self):
root_dir, base_dir = self._create_files()
base_name = os.path.join(self.mkdtemp(), 'archive')
with no_chdir:
tarball = make_archive(base_name, 'gztar', root_dir, base_dir)
tarball = self._make_archive(base_name, 'gztar', root_dir, base_dir)

# check if the compressed tarball was created
self.assertEqual(tarball, base_name + '.tar.gz')
Expand All @@ -1412,13 +1412,13 @@ def test_tarfile_vs_tar(self):

# trying an uncompressed one
with no_chdir:
tarball = make_archive(base_name, 'tar', root_dir, base_dir)
tarball = self._make_archive(base_name, 'tar', root_dir, base_dir)
self.assertEqual(tarball, base_name + '.tar')
self.assertTrue(os.path.isfile(tarball))

# now for a dry_run
with no_chdir:
tarball = make_archive(base_name, 'tar', root_dir, base_dir,
tarball = self._make_archive(base_name, 'tar', root_dir, base_dir,
dry_run=True)
self.assertEqual(tarball, base_name + '.tar')
self.assertTrue(os.path.isfile(tarball))
Expand All @@ -1437,7 +1437,7 @@ def test_make_zipfile(self):

with os_helper.change_cwd(work_dir), no_chdir:
base_name = os.path.abspath(rel_base_name)
res = make_archive(rel_base_name, 'zip', root_dir)
res = self._make_archive(rel_base_name, 'zip', root_dir)

self.assertEqual(res, base_name + '.zip')
self.assertTrue(os.path.isfile(res))
Expand All @@ -1450,7 +1450,7 @@ def test_make_zipfile(self):

with os_helper.change_cwd(work_dir), no_chdir:
base_name = os.path.abspath(rel_base_name)
res = make_archive(rel_base_name, 'zip', root_dir, base_dir)
res = self._make_archive(rel_base_name, 'zip', root_dir, base_dir)

self.assertEqual(res, base_name + '.zip')
self.assertTrue(os.path.isfile(res))
Expand All @@ -1467,7 +1467,7 @@ def test_zipfile_vs_zip(self):
root_dir, base_dir = self._create_files()
base_name = os.path.join(self.mkdtemp(), 'archive')
with no_chdir:
archive = make_archive(base_name, 'zip', root_dir, base_dir)
archive = self._make_archive(base_name, 'zip', root_dir, base_dir)

# check if ZIP file was created
self.assertEqual(archive, base_name + '.zip')
Expand All @@ -1494,7 +1494,7 @@ def test_unzip_zipfile(self):
root_dir, base_dir = self._create_files()
base_name = os.path.join(self.mkdtemp(), 'archive')
with no_chdir:
archive = make_archive(base_name, 'zip', root_dir, base_dir)
archive = self._make_archive(base_name, 'zip', root_dir, base_dir)

# check if ZIP file was created
self.assertEqual(archive, base_name + '.zip')
Expand All @@ -1515,7 +1515,7 @@ def test_unzip_zipfile(self):
def test_make_archive(self):
tmpdir = self.mkdtemp()
base_name = os.path.join(tmpdir, 'archive')
self.assertRaises(ValueError, make_archive, base_name, 'xxx')
self.assertRaises(ValueError, self._make_archive, base_name, 'xxx')

@support.requires_zlib()
def test_make_archive_owner_group(self):
Expand All @@ -1529,18 +1529,18 @@ def test_make_archive_owner_group(self):

root_dir, base_dir = self._create_files()
base_name = os.path.join(self.mkdtemp(), 'archive')
res = make_archive(base_name, 'zip', root_dir, base_dir, owner=owner,
res = self._make_archive(base_name, 'zip', root_dir, base_dir, owner=owner,
group=group)
self.assertTrue(os.path.isfile(res))

res = make_archive(base_name, 'zip', root_dir, base_dir)
res = self._make_archive(base_name, 'zip', root_dir, base_dir)
self.assertTrue(os.path.isfile(res))

res = make_archive(base_name, 'tar', root_dir, base_dir,
res = self._make_archive(base_name, 'tar', root_dir, base_dir,
owner=owner, group=group)
self.assertTrue(os.path.isfile(res))

res = make_archive(base_name, 'tar', root_dir, base_dir,
res = self._make_archive(base_name, 'tar', root_dir, base_dir,
owner='kjhkjhkjg', group='oihohoh')
self.assertTrue(os.path.isfile(res))

Expand All @@ -1553,7 +1553,7 @@ def test_tarfile_root_owner(self):
group = grp.getgrgid(0)[0]
owner = pwd.getpwuid(0)[0]
with os_helper.change_cwd(root_dir), no_chdir:
archive_name = make_archive(base_name, 'gztar', root_dir, 'dist',
archive_name = self._make_archive(base_name, 'gztar', root_dir, 'dist',
owner=owner, group=group)

# check if the compressed tarball was created
Expand Down Expand Up @@ -1582,7 +1582,7 @@ def _chdir(path):
try:
with support.swap_attr(os, 'chdir', _chdir) as orig_chdir:
try:
make_archive('xxx', 'xxx', root_dir=root_dir)
self._make_archive('xxx', 'xxx', root_dir=root_dir)
except Exception:
pass
self.assertEqual(os.getcwd(), current_dir)
Expand All @@ -1594,15 +1594,15 @@ def test_make_tarfile_in_curdir(self):
# Issue #21280
root_dir = self.mkdtemp()
with os_helper.change_cwd(root_dir), no_chdir:
self.assertEqual(make_archive('test', 'tar'), 'test.tar')
self.assertEqual(self._make_archive('test', 'tar'), 'test.tar')
self.assertTrue(os.path.isfile('test.tar'))

@support.requires_zlib()
def test_make_zipfile_in_curdir(self):
# Issue #21280
root_dir = self.mkdtemp()
with os_helper.change_cwd(root_dir), no_chdir:
self.assertEqual(make_archive('test', 'zip'), 'test.zip')
self.assertEqual(self._make_archive('test', 'zip'), 'test.zip')
self.assertTrue(os.path.isfile('test.zip'))

def test_register_archive_format(self):
Expand Down Expand Up @@ -1634,7 +1634,7 @@ def check_unpack_archive_with_converter(self, format, converter):
expected.remove('outer')

base_name = os.path.join(self.mkdtemp(), 'archive')
filename = make_archive(base_name, format, root_dir, base_dir)
filename = self._make_archive(base_name, format, root_dir, base_dir)

# let's try to unpack it now
tmpdir2 = self.mkdtemp()
Expand Down Expand Up @@ -1696,6 +1696,22 @@ def _boo(filename, extract_dir, extra):
self.assertEqual(get_unpack_formats(), formats)


class TestStrPathArchives(_ArchiveTests, unittest.TestCase):

def _make_archive_with_path(self, *args, **kwargs):
return make_archive(*args, **kwargs)

_make_archive = _make_archive_with_path


class TestPathlibPathArchives(_ArchiveTests, unittest.TestCase):

def _make_archive_with_path(self, path, /, *args, **kwargs):
return make_archive(pathlib.Path(path), *args, **kwargs)

_make_archive = _make_archive_with_path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to add a class. A single test will suffice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted; I agree that after the conversion moved into make_archive frontend, there is no need to explicitly test _make_* backends.



class TestMisc(BaseTest, unittest.TestCase):

@unittest.skipUnless(hasattr(shutil, 'disk_usage'),
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2022-07-14-10-28-10.gh-issue-94844.NJ3W80.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
:meth:`shutil.make_archive` now accepts any :class:`os.PathLike`. Patch by
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was undocumented behavior, and we do not yet guarantee it in future versions, so no NEWS entry is needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done; could you add skip-news label please?

Oleg Iarygin.