GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-65106: shutil._unpack_zipfile add warnings for skipped files by akulakov · Pull Request #29910 · python/cpython

Conversation

@akulakov

@akulakov akulakov commented Dec 4, 2021 •
edited by serhiy-storchaka
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Note that test zip only has a '..' path component for testing, and no absolute path. My zip archiver (as well as Python zip archiver) do not create absolute paths. It seems like all (?) modern zip archivers don't create absolute paths. If someone knows of an easy way to add such a path to the test zip file, I can update the unit test for that.

https://bugs.python.org/issue20907

  • Issue: behavioral differences between shutil.unpack_archive and ZipFile.extractall #65106

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
akulakov added 4 commits December 3, 2021 17:51
@akulakov
initial
d713ba8
@akulakov
add test; fix logic and add test zip file
5bde783
@akulakov
add news
14b7e08
@akulakov
move warnings import
6ca1160
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Dec 4, 2021
@bedevere-bot bedevere-bot added the awaiting review label Dec 4, 2021
akulakov added 2 commits December 3, 2021 19:38
@akulakov
add note to the docs
9ff16a2
@akulakov
fix test
54d3c4e
merwok
merwok reviewed Dec 4, 2021
View reviewed changes
Comment thread Lib/shutil.py
if skipped:
import warnings
warnings.warn(f'unpack {filename}: {skipped} file(s) skipped'
' (due to absolute path or `..` path component)')

@merwok merwok Dec 4, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

warnings are a mecanism for a module author to communicate about bad usage to other developers calling the code, not to communicate with end-users: https://docs.python.org/3/howto/logging.html#when-to-use-logging

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ambv reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@akulakov akulakov Dec 4, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

So using logging.warning() here should be appropriate?

Edit: I'm not really sure between logging.info, warnings.warn and logging.warning. This function can be used in both libraries and end user apps.

  • it's not necessarily true (but can be) that client app needs to be modified -- for example if it needs to always extract all files, it can be changed to use ZipFile module
  • it's also not true that client app cannot do anything about it -- it can potentially use ZipFIle module or 3rd party module or custom logic.
  • logging.info seems not specific enough because we're warning somebody (user or developer) that files are skipped.

This might be an edge case that falls between warnings.warn and logging.warning? If so, I don't mind using one that you prefer.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@akulakov akulakov Dec 4, 2021

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I looked for a similar usages in the library and found this:

cpython/Lib/zipfile.py

Line 1480 in f42a06b

if len(comment) > ZIP_MAX_COMMENT:

it's very similar in that both cases warn about something being lost when creating / unpacking a zip file: some contained files and a part of a comment respectively. That warning was added in 2014.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@merwok merwok Dec 5, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Good find! @serhiy-storchaka added that change, maybe he can comment here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@serhiy-storchaka serhiy-storchaka Dec 5, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I just replaced existing prints with warnings. I am not sure that it is a good solution, but it at least gives the user some control: warnings can be printed, silenced or converted to exceptions.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@akulakov akulakov Dec 6, 2021

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I found another example where warning would be for the end user rather than app author: https://github.com/pganssle/zoneinfo/blob/c54d700fcba78b20733390ee3014809ad69858f6/src/backports/zoneinfo/_tzpath.py#L58

-- this one is from 2020 by Paul Ganssle

However I think for this use case, I don't see any issue with using logging.warning or logging.error. But I wonder which one of those would be more appropriate. It fits the description of logging.error more closely but at the same time treating it as an "error" might be too strong.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@ambv ambv Dec 10, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

In general logging.warning would be cleaner here than using the warnings module. As Éric says, this is a data-driven problem that is of interest to the end-user, less so to the developer of the application.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@ambv ambv Dec 10, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@akulakov you could in fact ask on python-dev about the general practice if you want feedback; but I don't think this is particularly controversial: libraries can log things to end-users, and many do. I'd just use the logging module here, and as a follow-up task, fix the other warnings call you found in zipfile.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@akulakov akulakov Dec 11, 2021

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@ambv: I agree; I was going to update it but got a little distracted -- will push an update today or tomorrow.. Thanks for looking at this as well!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@akulakov akulakov Dec 11, 2021

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

A few functions in shutil.py accept logger as an arg and use it to print info/debug logs. I think in this case there's no reason to follow this pattern because we want the warnings to be displayed by default.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@akulakov
use logging.warning instead of warnings.warn
70c8f29
@akulakov

akulakov commented Dec 17, 2021

Copy link
Copy Markdown
Contributor Author

I've updated to use logging.warning, @merwok @ambv please take a look if you have time..

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MaxwellDupre
MaxwellDupre approved these changes Apr 15, 2022
View reviewed changes

@MaxwellDupre MaxwellDupre left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ran 164 tests in 2.291s
OK (skipped=24)
Looks ok.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Apr 15, 2022
@peterpsantoronet peterpsantoronet mannequin mentioned this pull request Apr 10, 2022
behavioral differences between shutil.unpack_archive and ZipFile.extractall #65106
Open
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@ambv
Merge branch 'main' into 20907-unpack_zipfile-add-warnings-for-skippe…
9336cff
…d-files
@serhiy-storchaka

serhiy-storchaka commented Dec 16, 2023

Copy link
Copy Markdown
Member

See also #111824 which fixes the original issue in a different way.

sepastian reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions

github-actions Bot commented Apr 10, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 10, 2026
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Aug 12, 2026
@serhiy-storchaka serhiy-storchaka changed the title bpo-20907: shutil._unpack_zipfile add warnings for skipped files gh-65106: shutil._unpack_zipfile add warnings for skipped files Aug 12, 2026
@github-actions

github-actions Bot commented Nov 12, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 90 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Nov 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review stale Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@akulakov @serhiy-storchaka @ambv @merwok @MaxwellDupre @the-knights-who-say-ni @ezio-melotti @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.