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-74696: Pass root_dir to custom archivers which support it by serhiy-storchaka · Pull Request #94251 · python/cpython

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Jun 25, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Issue: shutil.make_archive should not need to chdir (alternatively: make shutil.make_archive thread-safe) #74696

Sorry, something went wrong.

Uh oh!

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

All reactions
@serhiy-storchaka
pythongh-74696: Pass root_dir to custom archivers which support it
e5ebb8d
@serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Jun 25, 2022
@serhiy-storchaka
serhiy-storchaka requested review from ambv and gpshead June 25, 2022 06:16
@bedevere-bot bedevere-bot added the awaiting core review label Jun 25, 2022
@serhiy-storchaka serhiy-storchaka mentioned this pull request Jun 25, 2022
shutil.make_archive should not need to chdir (alternatively: make shutil.make_archive thread-safe) #74696
Closed
merwok
merwok reviewed Jun 25, 2022
View reviewed changes
Comment thread Doc/library/shutil.rst Resolved

Uh oh!

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

@serhiy-storchaka @merwok
Update Doc/library/shutil.rst
563b0f5
Co-authored-by: Éric <[email protected]>
@ambv

ambv commented Jul 1, 2022

Copy link
Copy Markdown
Contributor

@serhiy-storchaka, since this is 3.12+, would it be possible to use inspect.signature() instead of relying on a new attribute on the function (supports_root_dir)? Pure Python functions, as well as Cython functions, work fine with inspect.signature().

All reactions

Sorry, something went wrong.

Uh oh!

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

@serhiy-storchaka

serhiy-storchaka commented Jul 4, 2022

Copy link
Copy Markdown
Member Author

I do not like relying on inspect. It fails in too many cases. What if the function signature contains **kwargs? inspect looks to me rather as an inspecting and debugging tool.

zooba 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.

Éric
Merge branch 'main' into shutil-register_archive_format-supports_root…
2190304
…_dir-attr
@gpshead

gpshead commented Oct 1, 2022

Copy link
Copy Markdown
Member

Setting a magic attribute on a function is always an awkward API. Why not just add a keyword only argument to the register_*_format() function to declare whether the supplied archiver callable supports a root_dir kwarg or not?

All reactions

Sorry, something went wrong.

Uh oh!

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

gpshead
gpshead reviewed Oct 1, 2022
View reviewed changes
Comment thread Lib/test/test_shutil.py Resolved

Uh oh!

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

NoSuck and others added 17 commits October 2, 2022 22:38
@NoSuck @serhiy-storchaka
closes pythongh-97650: correct sphinx executable (pythongh-97651)
7dc8fb3
@jeff5 @CAM-Gerlach @serhiy-storchaka
pythongh-96397: Document that attributes need not be identifiers (pyt…
679cf96
…hon#96454)

Co-authored-by: C.A.M. Gerlach <[email protected]>
@ofey404 @serhiy-storchaka
pythongh-96348: Deprecate the 3-arg signature of coroutine.throw and …
3c84af2
…generator.throw (pythonGH-96428)
@ehebert @serhiy-storchaka
Use SyntaxError invalid range in tutorial introduction example (pytho…
8a0ad46
…nGH-93031)

Use output from a 3.10+ REPL, showing invalid range, for the
SyntaxError examples in the tutorial introduction page.

Automerge-Triggered-By: GH:iritkatriel
@zooba @serhiy-storchaka
pythongh-97649: The Tools directory is no longer installed on Windows (…
182755f
…pythonGH-97653)
@zooba @serhiy-storchaka
pythongh-90989: Install Windows launcher per-user, and clarify some i…
b1a9de0
…nstaller text (pythonGH-97655)
@vstinner @serhiy-storchaka
pythongh-94526: getpath_dirname() no longer encodes the path (python#…
cc1e8e0
…97645)

Fix the Python path configuration used to initialized sys.path at
Python startup. Paths are no longer encoded to UTF-8/strict to avoid
encoding errors if it contains surrogate characters (bytes paths are
decoded with the surrogateescape error handler).

getpath_basename() and getpath_dirname() functions no longer encode
the path to UTF-8/strict, but work directly on Unicode strings. These
functions now use PyUnicode_FindChar() and PyUnicode_Substring() on
the Unicode path, rather than strrchr() on the encoded bytes string.
@csabella @terryjreedy @serhiy-storchaka
bpo-35675: IDLE - separate config_key window and frame (python#11427)
8fd0e86
bpo-35598: IDLE: Refactor window and frame class

Co-authored-by: Terry Jan Reedy <[email protected]>
@gpshead @serhiy-storchaka
pythongh-87597: Document TimeoutExpired.stdout & .stderr types (pytho…
d1d6b31
…n#97685)

This documents the behavior that has always been the case since timeout
support was introduced in Python 3.3.
@gvanrossum @serhiy-storchaka
pythonGH-96827: Don't touch closed loops from executor threads (pytho…
c6203f8
…n#96837)

* When chaining futures, skip callback if loop closed.
* When shutting down an executor, don't wake a closed loop.
@gvanrossum @serhiy-storchaka
pythonGH-97592: Fix crash in C remove_done_callback due to evil code (p…
67851dc
…ython#97660)

Evil code could cause fut_callbacks to be cleared when PyObject_RichCompareBool is called.
@ericsnowcurrently @serhiy-storchaka
pythongh-90110: Update the c-analyzer Tool (pythongh-97695)
4c95e50
python#90110
@ambv @graingert @serhiy-storchaka
pythongh-90908: Document asyncio.Task.cancelling() and asyncio.Task.u…
fb39e7f
…ncancel() (python#95253)

Co-authored-by: Thomas Grainger <[email protected]>
@hawkinsw @serhiy-storchaka
Fix capitalization of Unix in documentation (python#96913)
0bb7e71
@gpshead @serhiy-storchaka
pythongh-95588: Drop the safety claim from ast.literal_eval docs. (p…
5ffc011
…ython#95919)

It was never really safe and this claim conflicts directly with the big warning in the docs about it being able to crash the interpreter.
@ofey404 @serhiy-storchaka
pythongh-97591: In Exception.__setstate__() acquire strong referenc…
e401b65
…es before calling `tp_hash` slot (python#97700)
@CAM-Gerlach @ezio-melotti @serhiy-storchaka
pythongh-95975: Move except/*/finally ref labels to more precise loca…
879e866
…tions (python#95976)

* pythongh-95975: Move except/*/finally ref labels to more precise locations

* Add section headers to fix :keyword: role and aid navigation

* Move see also to the introduction rather than a particular subsection

* Fix other minor Sphinx syntax issues with except

Co-authored-by: Ezio Melotti <[email protected]>

* Suppress redundant link to same section for except too

* Don't link try/except/else/finally keywords if in the same section

* Format try/except/finally as keywords in modified sections

Co-authored-by: Ezio Melotti <[email protected]>
@serhiy-storchaka
serhiy-storchaka removed request for 1st1, asvetlov, gvanrossum, iritkatriel, isidentical, markshannon and terryjreedy October 2, 2022 19:42
@serhiy-storchaka

serhiy-storchaka commented Oct 2, 2022

Copy link
Copy Markdown
Member Author

Setting a magic attribute on a function is always an awkward API. Why not just add a keyword only argument to the register_*_format() function to declare whether the supplied archiver callable supports a root_dir kwarg or not?

I considered this option. But it would make the caller of shutil.register_archive_format() responsible for passing the necessary option. For example, if py7zr add support for root_dir, the users of py7zr will need to replace shutil.register_archive_format('7zip', pack_7zarchive, description='7zip archive') in their code with shutil.register_archive_format('7zip', pack_7zarchive, description='7zip archive', supports_root_dir=True). Since it will only works on Python 3.12+, the code will be more complicated. Users which will not do this, will still have the working code, but it is not safe.

In future we can deprecate archivers which does not support root_dir. And finally raise an error if shutil.register_archive_format() is called without supports_root_dir=True. Since that this option will became useless but mandatory, and several versions later we could deprecate and finally remove it. Thus, users will be first forced to add this option (first conditionally, then unconditionally), them remove it. And users which skip versions will risk to get incorrectly working program (using an archiver which does not support root_dir with the root_dir argument).

This all will take very long time until we came to new equilibrium, will cause headache to users, and will be errorprone.

With the variant implemented in this PR, the author of the archiver is responsible for setting the supports_root_dir attribute if the archiver supports the root_dir argument. The user's code will not change.

merwok, zooba, and gpshead reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

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

gvanrossum
gvanrossum approved these changes Oct 4, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

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.

Just two doc nits, entirely optional.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Doc/library/shutil.rst Resolved

Uh oh!

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

Comment thread Doc/library/shutil.rst Resolved

Uh oh!

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

@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Oct 4, 2022
merwok
merwok reviewed Oct 4, 2022
View reviewed changes
Comment thread Doc/library/shutil.rst Resolved

Uh oh!

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

merwok
merwok reviewed Oct 4, 2022
View reviewed changes
Comment thread Doc/library/shutil.rst Resolved

Uh oh!

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

@gvanrossum gvanrossum mentioned this pull request Oct 4, 2022
shutil.make_archive now fails with pathlib.Path object in Python 3.11.0b4 #94844
Closed
@serhiy-storchaka @merwok
Apply suggestions from code review
9980c8c
Co-authored-by: Éric <[email protected]>
merwok
merwok reviewed Oct 4, 2022
View reviewed changes
Comment thread Doc/library/shutil.rst Resolved

Uh oh!

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

@merwok
fix markup
5160cb7
gvanrossum
gvanrossum approved these changes Oct 4, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

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.

Excellent. @serhiy-storchaka feel free to land!

Sorry, something went wrong.

Uh oh!

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

All reactions
gpshead
gpshead approved these changes Oct 4, 2022
View reviewed changes
@gpshead gpshead added sprint stdlib Standard Library Python modules in the Lib/ directory labels Oct 4, 2022
serhiy-storchaka
serhiy-storchaka commented Oct 5, 2022
View reviewed changes
Comment thread Doc/whatsnew/3.12.rst Resolved

Uh oh!

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

@serhiy-storchaka
Update Doc/whatsnew/3.12.rst
701f896
@serhiy-storchaka
serhiy-storchaka merged commit e3ef400 into python:main Oct 5, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Oct 5, 2022
@serhiy-storchaka
serhiy-storchaka deleted the shutil-register_archive_format-supports_root_dir-attr branch October 5, 2022 09:49
@serhiy-storchaka

serhiy-storchaka commented Oct 5, 2022

Copy link
Copy Markdown
Member Author

Thank you for your review and suggestions.

gpshead reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

carljm added a commit to carljm/cpython that referenced this pull request Oct 6, 2022
@carljm
Merge branch 'main' into dictwatch
4bb5d9d
* main: (66 commits)
  pythongh-65961: Raise `DeprecationWarning` when `__package__` differs from `__spec__.parent` (python#97879)
  docs(typing): add "see PEP 675" to LiteralString (python#97926)
  pythongh-97850: Remove all known instances of module_repr() (python#97876)
  I changed my surname early this year (python#96671)
  pythongh-93738: Documentation C syntax (:c:type:<C type> -> :c:expr:<C type>) (python#97768)
  pythongh-91539: improve performance of get_proxies_environment  (python#91566)
  build(deps): bump actions/stale from 5 to 6 (python#97701)
  pythonGH-95172 Make the same version `versionadded` oneline (python#95172)
  pythongh-88050: Fix asyncio subprocess to kill process cleanly when process is blocked (python#32073)
  pythongh-93738: Documentation C syntax (Function glob patterns -> literal markup) (python#97774)
  pythongh-93357: Port test cases to IsolatedAsyncioTestCase, part 2 (python#97896)
  pythongh-95196: Disable incorrect pickling of the C implemented classmethod descriptors (pythonGH-96383)
  pythongh-97758: Fix a crash in getpath_joinpath() called without arguments (pythonGH-97759)
  pythongh-74696: Pass root_dir to custom archivers which support it (pythonGH-94251)
  pythongh-97661: Improve accuracy of sqlite3.Cursor.fetchone docs (python#97662)
  pythongh-87092: bring compiler code closer to a preprocessing-opt-assembler organisation (pythonGH-97644)
  pythonGH-96704: Add {Task,Handle}.get_context(), use it in call_exception_handler() (python#96756)
  pythongh-93738: Documentation C syntax (:c:type:`PyTypeObject*` -> :c:expr:`PyTypeObject*`) (python#97778)
  pythongh-97825: fix AttributeError when calling subprocess.check_output(input=None) with encoding or errors args (python#97826)
  Add re.VERBOSE flag documentation example (python#97678)
  ...
mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022
@serhiy-storchaka @merwok @mpage
pythongh-74696: Pass root_dir to custom archivers which support it (p…
c34bef6
…ythonGH-94251)

Co-authored-by: Éric <[email protected]>
@serhiy-storchaka serhiy-storchaka mentioned this pull request Feb 20, 2024
gh-115398: Expose Expat >=2.6.0 reparse deferral API (CVE-2023-52425) #115623
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merwok merwok merwok left review comments
@gpshead gpshead gpshead approved these changes
@gvanrossum gvanrossum gvanrossum approved these changes
@ambv ambv Awaiting requested review from ambv

Uh oh!

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

Assignees

No one assigned

Labels

sprint stdlib Standard Library Python modules in the Lib/ directory type-feature A feature request or enhancement

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.

17 participants

@serhiy-storchaka @ambv @gpshead @merwok @gvanrossum @bedevere-bot @NoSuck @jeff5 @ofey404 @ehebert @zooba @vstinner @csabella @ericsnowcurrently @hawkinsw @CAM-Gerlach @vsajip
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.