Skip to content

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

Merged
serhiy-storchaka merged 12 commits into
python:mainfrom
arhadthedev:fix-shutil-makearchive
Jul 20, 2022
Merged

gh-94844: Add pathlib support to shutil archive management#94846
serhiy-storchaka merged 12 commits into
python:mainfrom
arhadthedev:fix-shutil-makearchive

Conversation

@arhadthedev

@arhadthedev arhadthedev commented Jul 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Underlying ZipFile.__init__ casts a path to str anyway:

cpython/Lib/zipfile.py

Lines 1270 to 1271 in b03a9e8

if isinstance(file, os.PathLike):
file = os.fspath(file)

so make_archive can safely cast its path param to a str as well (including implicit conversion in f-strings).

@arhadthedev arhadthedev changed the title gh-94844: Fix shutil makearchive gh-94844: Add pathlib support to shutil archive management Jul 14, 2022
@barneygale

barneygale commented Jul 14, 2022

Copy link
Copy Markdown
Contributor

make_archive() could be adjusted to call base_name = os.fspath(base_name).

@arhadthedev arhadthedev marked this pull request as draft July 14, 2022 08:33
@arhadthedev arhadthedev marked this pull request as ready for review July 14, 2022 08:44
@arhadthedev

arhadthedev commented Jul 14, 2022

Copy link
Copy Markdown
Member Author

@barneygale Thank you, addressed.

@barneygale

barneygale commented Jul 14, 2022

Copy link
Copy Markdown
Contributor

Could you call os.fspath() in make_archive() rather than _make_zipfile() and _make_tarball()?

@arhadthedev

arhadthedev commented Jul 14, 2022

Copy link
Copy Markdown
Member Author

Fixed; I quiet misunderstood first.

Currently I put the conversion after sys.audit to not break backward compatibility and provide audit listeners raw input data. I can change this if needed.

@barneygale

barneygale commented Jul 14, 2022

Copy link
Copy Markdown
Contributor

Could you remove the changes to _make_zipfile() and _make_tarball() please?

barneygale
barneygale reviewed Jul 14, 2022
View reviewed changes
Comment thread Lib/test/test_shutil.py Outdated
Comment on lines +1707 to +1712
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

@barneygale barneygale Jul 14, 2022

Copy link
Copy Markdown
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.

@arhadthedev arhadthedev Jul 14, 2022

Copy link
Copy Markdown
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.

serhiy-storchaka
serhiy-storchaka reviewed Jul 16, 2022
View reviewed changes
Comment thread Lib/shutil.py Outdated
if not dry_run:
os.chdir(root_dir)

base_name = os.fspath(base_name)

@serhiy-storchaka serhiy-storchaka Jul 16, 2022

Copy link
Copy Markdown
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.

@arhadthedev arhadthedev Jul 16, 2022

Copy link
Copy Markdown
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.

Comment thread Misc/NEWS.d/next/Library/2022-07-14-10-28-10.gh-issue-94844.NJ3W80.rst Outdated
@@ -0,0 +1,2 @@
:meth:`shutil.make_archive` now accepts any :class:`os.PathLike`. Patch by

@serhiy-storchaka serhiy-storchaka Jul 16, 2022

Copy link
Copy Markdown
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.

@arhadthedev arhadthedev Jul 16, 2022

Copy link
Copy Markdown
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?

barneygale
barneygale reviewed Jul 16, 2022
View reviewed changes
Comment thread Lib/shutil.py Outdated
@serhiy-storchaka serhiy-storchaka merged commit ed44415 into python:main Jul 20, 2022
@serhiy-storchaka serhiy-storchaka added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Jul 20, 2022
@miss-islington

miss-islington commented Jul 20, 2022

Copy link
Copy Markdown
Contributor

Thanks @arhadthedev for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington

miss-islington commented Jul 20, 2022

Copy link
Copy Markdown
Contributor

Thanks @arhadthedev for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot

bedevere-bot commented Jul 20, 2022

Copy link
Copy Markdown

GH-95054 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jul 20, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 20, 2022
@arhadthedev @miss-islington
pythongh-94844: Add pathlib support to shutil archive management (pyt…
cadf202 
…honGH-94846)

Co-authored-by: Barney Gale <barney.gale@gmail.com>
(cherry picked from commit ed44415)

Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 20, 2022
@arhadthedev @miss-islington
pythongh-94844: Add pathlib support to shutil archive management (pyt…
c7d812d 
…honGH-94846)

Co-authored-by: Barney Gale <barney.gale@gmail.com>
(cherry picked from commit ed44415)

Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jul 20, 2022
@bedevere-bot

bedevere-bot commented Jul 20, 2022

Copy link
Copy Markdown

GH-95055 is a backport of this pull request to the 3.11 branch.

@arhadthedev

arhadthedev commented Jul 20, 2022

Copy link
Copy Markdown
Member Author

@serhiy-storchaka Thanks for merging.

@arhadthedev arhadthedev deleted the fix-shutil-makearchive branch July 20, 2022 16:01
miss-islington added a commit that referenced this pull request Jul 20, 2022
@miss-islington @arhadthedev
gh-94844: Add pathlib support to shutil archive management (GH-94846)
6cc9489 
Co-authored-by: Barney Gale <barney.gale@gmail.com>
(cherry picked from commit ed44415)

Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
miss-islington added a commit that referenced this pull request Jul 20, 2022
@miss-islington @arhadthedev
gh-94844: Add pathlib support to shutil archive management (GH-94846)
cbe3ad5 
Co-authored-by: Barney Gale <barney.gale@gmail.com>
(cherry picked from commit ed44415)

Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
@gvanrossum gvanrossum mentioned this pull request Oct 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@barneygale barneygale barneygale left review comments
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
+1 more reviewer
@bateller bateller bateller approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@arhadthedev @barneygale @miss-islington @bedevere-bot @serhiy-storchaka @bateller @AlexWaygood