Skip to content
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.

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

Jump to bottom

shutil.make_archive should not need to chdir (alternatively: make shutil.make_archive thread-safe) #74696

Open
AlexGaynor mannequin opened this issue May 30, 2017 · 18 comments
Open

shutil.make_archive should not need to chdir (alternatively: make shutil.make_archive thread-safe) #74696

AlexGaynor mannequin opened this issue May 30, 2017 · 18 comments
Assignees
@serhiy-storchaka
Labels
stdlib type-bug type-feature

Comments

@AlexGaynor
Copy link
Mannequin

@AlexGaynor AlexGaynor mannequin commented May 30, 2017

BPO 30511
Nosy @tarekziade, @ambv, @serhiy-storchaka, @miss-islington, @michael-o, @FFY00, @akulakov
PRs
  • #26933
  • #27274
  • #27275
  • #27276
  • #28271
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = None
created_at = <Date 2017-05-30.13:46:18.840>
labels = ['3.7', 'library']
title = 'shutil.make_archive should not need to chdir (alternatively: make shutil.make_archive thread-safe)'
updated_at = <Date 2021-09-17.13:26:13.713>
user = 'https://bugs.python.org/AlexGaynor'

    bugs.python.org fields:

    activity = <Date 2021-09-17.13:26:13.713>
actor = 'FFY00'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2017-05-30.13:46:18.840>
creator = 'Alex Gaynor'
dependencies = []
files = []
hgrepos = []
issue_num = 30511
keywords = ['patch']
message_count = 10.0
messages = ['294756', '294758', '294759', '294760', '297902', '389849', '396633', '397928', '397932', '397934', '397935']
nosy_count = 9.0
nosy_names = ['tarek', 'lukasz.langa', 'serhiy.storchaka', 'Alex Gaynor', 'Joey Harrington', 'miss-islington', 'michael-o', 'FFY00', 'andrei.avk']
pr_nums = ['26933', '27274', '27275', '27276', '28271']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue30511'
versions = ['Python 2.7', 'Python 3.7']
    @AlexGaynor
    Copy link
    Mannequin Author

    @AlexGaynor AlexGaynor mannequin commented May 30, 2017

    Currently shutil.make_archive uses os.chdir, however there's no need for that. Everything that's done could be equally accomplished with path manipulation:

    https://github.com/python/cpython/blob/master/Lib/shutil.py#L773-L779

    We should switch to using path manipulation in order to make shutil.make_archive thread safe.

    (Flag: This is probably a good bug for someone with Python skills interested in contributing to CPython!)

    @AlexGaynor AlexGaynor mannequin added 3.7 stdlib labels May 30, 2017
    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented May 30, 2017

    Unfortunately this is not possible. The signature of functions registered with register_archive_format() doesn't support this.

    @AlexGaynor
    Copy link
    Mannequin Author

    @AlexGaynor AlexGaynor mannequin commented May 30, 2017

    None of those functions are a public API, so changing them shouldn't be a problem IMO.

    @AlexGaynor
    Copy link
    Mannequin Author

    @AlexGaynor AlexGaynor mannequin commented May 30, 2017

    Ugh, except via |register_archive_format|. |register_archive_format| could wrap callables passed to it to maintain the current behavior.

    @JoeyHarrington
    Copy link
    Mannequin

    @JoeyHarrington JoeyHarrington mannequin commented Jul 7, 2017

    It would be nice if there was at least a warning in the docs that make_archive is not thread-safe, and that if you have two threads creating archives that it's extremely likely you'll get erroneous results since the race condition lasts for the entire duration of the archive creation.

    @michael-o
    Copy link
    Mannequin

    @michael-o michael-o mannequin commented Mar 30, 2021

    Just wasted two hours for this. Can someone really update the documentation of it if this is not going to change.

    @akulakov
    Copy link
    Contributor

    @akulakov akulakov commented Jun 28, 2021

    PR is added here:
    #26933

    @ambv
    Copy link
    Contributor

    @ambv ambv commented Jul 21, 2021

    New changeset 64f54b7 by andrei kulakov in branch 'main':
    bpo-30511: Add note on thread safety to shutil.make_archive() (bpo-26933)
    64f54b7

    @ambv
    Copy link
    Contributor

    @ambv ambv commented Jul 21, 2021

    New changeset d17449f by Miss Islington (bot) in branch '3.10':
    bpo-30511: Add note on thread safety to shutil.make_archive() (GH-26933) (GH-27274)
    d17449f

    @ambv
    Copy link
    Contributor

    @ambv ambv commented Jul 21, 2021

    New changeset c8e35ab by Miss Islington (bot) in branch '3.9':
    bpo-30511: Add note on thread safety to shutil.make_archive() (GH-26933) (GH-27275)
    c8e35ab

    @ambv
    Copy link
    Contributor

    @ambv ambv commented Jul 21, 2021

    New changeset 54d3871 by Miss Islington (bot) in branch '3.8':
    bpo-30511: Add note on thread safety to shutil.make_archive() (GH-26933) (bpo-27276)
    54d3871

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @gpshead
    Copy link
    Member

    @gpshead gpshead commented May 23, 2022

    Just a note that this is still relevant. We still get Python users at work who run into this. (the documentation definitely helps, but often after the fact while debugging a WTF?! style failure)

    I wouldn't let register_archive_format() et. al. hold us back. Such APIs can be deprecated with a new one requiring people to provide a thread safe archiver.

    We can detect when we're using an unknown externally registered format that is not chdir safe (anything unknown that came to us via register_archive_format()). We could add a make_archive() bool keyword argument thread_safe=False as a 3.12 feature to make attempts to use such an archiver raise an exception when True. We'd want to cleanup our existing built-in archiver functions to be thread safe ones as part of this.

    @gpshead gpshead added type-bug type-feature and removed 3.7 labels May 23, 2022
    @gpshead
    Copy link
    Member

    @gpshead gpshead commented May 23, 2022

    Also a reality check: What code exists that uses register_archive_format() in the wild? I find no uses in our huge internal codebase that included copies of many pypi/github OSS things. (surprising, usually there's at least 1 for anything no matter how esoteric it seems) This could be a very smooth deprecation process given so few apparent users...

    @serhiy-storchaka serhiy-storchaka self-assigned this May 24, 2022
    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented May 24, 2022

    Okay, I take this.

    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue May 24, 2022
    @serhiy-storchaka
    pythongh-74696: Do not change the current working directory when crea…
    49cc0a2 
    …te a zip or tar archive in shutil.make_archive()

It is still changed for custom archivers registered with shutil.register_archive_format()
if root_dir is not None.
    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue May 24, 2022
    @serhiy-storchaka
    pythongh-74696: Do not change the current working directory in shutil…
    d9f680d 
    ….make_archive() if possible

It is no longer changed when create a zip or tar archive.

It is still changed for custom archivers registered with shutil.register_archive_format()
if root_dir is not None.
    @serhiy-storchaka serhiy-storchaka mentioned this issue May 24, 2022
    Merged
    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented May 24, 2022

    #93160 is a first step. It makes standard zip and tar archivers not changing the current working directory. In the following PR I'll make it supporting in custom archivers.

    miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 22, 2022
    @serhiy-storchaka @miss-islington
    pythongh-74696: Do not change the current working directory in shutil…
    085dabd 
    ….make_archive() if possible (pythonGH-93160)

It is no longer changed when create a zip or tar archive.

It is still changed for custom archivers registered with shutil.register_archive_format()
if root_dir is not None.

Co-authored-by: Éric <merwok@netwok.org>
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit fda4b2f06364ae5ef91ecd9c09e2af380c8b0b4c)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    ambv added a commit that referenced this issue Jun 22, 2022
    @serhiy-storchaka @merwok @ambv
    gh-74696: Do not change the current working directory in shutil.make_…
    fda4b2f 
    …archive() if possible (GH-93160)

It is no longer changed when create a zip or tar archive.

It is still changed for custom archivers registered with shutil.register_archive_format()
if root_dir is not None.

Co-authored-by: Éric <merwok@netwok.org>
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
    miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 22, 2022
    @serhiy-storchaka @miss-islington
    pythongh-74696: Do not change the current working directory in shutil…
    d6823a8 
    ….make_archive() if possible (pythonGH-93160)

It is no longer changed when create a zip or tar archive.

It is still changed for custom archivers registered with shutil.register_archive_format()
if root_dir is not None.

Co-authored-by: Éric <merwok@netwok.org>
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit fda4b2f06364ae5ef91ecd9c09e2af380c8b0b4c)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    ambv pushed a commit that referenced this issue Jun 22, 2022
    @miss-islington @serhiy-storchaka
    gh-74696: Do not change the current working directory in shutil.make_…
    c1bfff4 
    …archive() if possible (GH-93160) (GH-94105)

It is no longer changed when create a zip or tar archive.

It is still changed for custom archivers registered with shutil.register_archive_format()
if root_dir is not None.

Co-authored-by: Éric <merwok@netwok.org>
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit fda4b2f)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    ambv pushed a commit that referenced this issue Jun 22, 2022
    @miss-islington @serhiy-storchaka
    gh-74696: Do not change the current working directory in shutil.make_…
    e51a22b 
    …archive() if possible (GH-93160) (GH-94106)

It is no longer changed when create a zip or tar archive.

It is still changed for custom archivers registered with shutil.register_archive_format()
if root_dir is not None.

Co-authored-by: Éric <merwok@netwok.org>
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit fda4b2f)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    @ambv
    Copy link
    Contributor

    @ambv ambv commented Jun 22, 2022

    @gpshead I'm +1 to deprecate register_archive_format() in 3.12 and remove in 𝛑-Py. Can we use this issue for that or should I open a new one?

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jun 22, 2022

    I was going to extend register_archive_format() by adding support of custom unpackers which support root_dir.

    It can be implemented either by adding a boolean parameter supports_root_dir in register_archive_format(), or checking the supports_root_dir attribute of the unpacker function.

    @ambv
    Copy link
    Contributor

    @ambv ambv commented Jun 22, 2022

    Sure, Serhiy. Greg's point is that this functionality looks unused so it's probably an unnecessary support burden.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @serhiy-storchaka serhiy-storchaka

    Labels
    stdlib type-bug type-feature
    Projects
    Threading issues
    Status: Todo
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @ambv @gpshead @akulakov @serhiy-storchaka