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 — bpo-14243: Optionally delete NamedTemporaryFile on content manager exit but not on file close by Ev2geny · Pull Request #22431 · python/cpython

Conversation

@Ev2geny

@Ev2geny Ev2geny commented Sep 27, 2020 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

This fixes this issue: https://bugs.python.org/issue14243

tempfile.NamedTemporaryFile is too hard to use portably when you need to open the file by name after writing it. To do that, you need to close the file first (on Windows), which means you have to pass delete=False, which in turn means that you get no help in cleaning up the actual file resource,

Hence at the moment there is no out of the box solution to use tempfile.NamedTemporaryFile on Windows in such scenario (which is often used in unit testing):

  • in test module:
  1. create and open temporary file
  2. write data to it
  3. pass name of the temporary file to the operational code
  • In operational code, being tested
  1. open file, using name of the temporary file
  2. read data from this temporary file

In this Pull Request the issue is solved by adding an additional optional argument to NamedTemporaryFile() 'delete_on_close' (default is True). It works in combination with already existing argument 'delete', and determines whether created temporary file gets deleted on close (which until this change was the only functionality available and it remains default now).

If 'delete' = True and 'delete_on_close' = False then temporary file gets deleted on context manager exit only (which is a new functionality).

If 'delete' is not true, then value of 'delete_on_close' is ignored.

So, the change shall be fully backwards compatible.

In the bug discussion such option was discussed by r.david.murray but mainly by eryksun

I have also added several unit tests to test new functionality in different combinations

https://bugs.python.org/issue14243

Sorry, something went wrong.

Uh oh!

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

vbrozik reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@the-knights-who-say-ni

the-knights-who-say-ni commented Sep 27, 2020

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@Ev2geny

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

All reactions

Sorry, something went wrong.

Uh oh!

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

@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Sep 27, 2020
@bedevere-bot bedevere-bot added the awaiting review label Sep 27, 2020
@Ev2geny Ev2geny changed the title bro-14243: Optionally close NamedTemporaryFile on content manager exit and not on close bro-14243: Optionally close NamedTemporaryFile on content manager exit and not on file close Sep 27, 2020
@Ev2geny Ev2geny changed the title bro-14243: Optionally close NamedTemporaryFile on content manager exit and not on file close bro-14243: Optionally delete NamedTemporaryFile on content manager exit and not on file close Sep 28, 2020
@Ev2geny Ev2geny changed the title bro-14243: Optionally delete NamedTemporaryFile on content manager exit and not on file close bro-14243: tempfile.NamedTemporaryFile not particularly useful on Windows Sep 28, 2020
@Ev2geny

Ev2geny commented Sep 28, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

I just can't figure out why bedevere/issue-number check is failing. Where do I have to mention it?

All reactions

Sorry, something went wrong.

Uh oh!

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

@Ev2geny Ev2geny changed the title bro-14243: tempfile.NamedTemporaryFile not particularly useful on Windows bro-14243: Optionally delete NamedTemporaryFile on content manager exit but not on file close Sep 28, 2020
@the-knights-who-say-ni the-knights-who-say-ni added CLA signed and removed CLA not signed labels Sep 29, 2020
@Chary314 Chary314 mentioned this pull request Oct 5, 2020
A lot of unittests tests fail under Windows due to usage of NamedTemporaryFile beancount/beancount#550
Open
@blais

blais commented Oct 11, 2020

Copy link
Copy Markdown
Contributor

Hmm, I'm not sure I like the API in this solution. If I want to write portable code (Linux and Windows), I would then have to use delete=True and delete_on_close=False in every since call, right? This is going to look at bit convoluted. I'd much rather this was a distinct type of NamedTemporaryFile.

All reactions

Sorry, something went wrong.

Uh oh!

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

@Ev2geny

Ev2geny commented Oct 12, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Hmm, I'm not sure I like the API in this solution. If I want to write portable code (Linux and Windows), I would then have to use delete=True and delete_on_close=False in every since call, right? This is going to look at bit convoluted. I'd much rather this was a distinct type of NamedTemporaryFile.

@blais , yes, this PR assumes, you would need to use delete_on_close=False (delete=True is already a default), if you want to write a portable code (Linux and Windows), but only if you need to refer to a created temporary file by its name, not by a file object. And this is a new functionality all together, as current functionality of the NamedTemporaryFile implicitly assumes, that you need to refer to a file by its file object (at least on Windows). Though I agree with you, that in test modules, one needs to often refer to temporary file by its name (hence the PR).

There were a lot of suggestions on how to fix this in the original issue14243 discussion with no clear conclusion on what is the best approach. I have chosen this one as it is backwards compatible and does not change philosophy of existing code. I will wait for code reviews to provide their feedback. There must be someone, who takes a final decision on this.

All reactions

Sorry, something went wrong.

Uh oh!

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

@blais

blais commented Oct 13, 2020

Copy link
Copy Markdown
Contributor

It would be wiser to create an alternative interface for this that is safe to use between platforms by default, even if it is more constrained, even if that means providing a completely different context manager.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bozhinov

bozhinov commented Oct 18, 2020

Copy link
Copy Markdown

OK. So I read the comments in the bug and the workaround here.
Here is what I did though (on Windows) - Created a temporary directory - put my temp file in there - script ends - all is gone
Hope that helps

All reactions

Sorry, something went wrong.

Uh oh!

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

@encukou encukou changed the title bro-14243: Optionally delete NamedTemporaryFile on content manager exit but not on file close bpo-14243: Optionally delete NamedTemporaryFile on content manager exit but not on file close Dec 14, 2020
@willingc

willingc commented Dec 15, 2020

Copy link
Copy Markdown
Contributor

@zooba Do you have time to take a look at this PR?

All reactions

Sorry, something went wrong.

Uh oh!

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

@Ev2geny

Ev2geny commented Jan 13, 2021

Copy link
Copy Markdown
Contributor Author

@zooba , just checking whether you managed to look at this

All reactions

Sorry, something went wrong.

Uh oh!

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

@dabrahams dabrahams mannequin mentioned this pull request Apr 10, 2022
tempfile.NamedTemporaryFile not particularly useful on Windows #58451
Closed
@MaxwellDupre

MaxwellDupre commented May 21, 2022

Copy link
Copy Markdown
Contributor

Can you re-target for 3.12 (Doc/whatsnew/3.10.rst) please?
Also, in NEWS I don't think you need double back ticks (``).

All reactions

Sorry, something went wrong.

Uh oh!

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

@AlexWaygood

AlexWaygood commented May 21, 2022

Copy link
Copy Markdown
Member

Also, in NEWS I don't think you need double back ticks (``).

@MaxwellDupre this is incorrect. Double backticks are usually preferred over single backticks in .rst files, unless a specific ReST role is being used.

Please read https://devguide.python.org/documenting/ for a full guide on ReST syntax.

erlend-aasland 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.

@kumaraditya303
kumaraditya303 requested review from eryksun and zooba May 21, 2022 11:27
zooba
zooba requested changes May 23, 2022
View reviewed changes

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

Code change looks good, but we need to get the docs in order.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/test/test_tempfile.py Resolved

Uh oh!

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

Comment thread Misc/NEWS.d/next/Library/2020-09-28-04-56-04.bpo-14243.YECnxv.rst Outdated

@zooba zooba May 23, 2022

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.

This is way more details than we want here - this will end up in a list with 1000 other changes.

Most of this should move to specific documentation, and then this entry can link to it with :class:`NamedTemporaryFile`

Sorry, something went wrong.

Uh oh!

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

All reactions

@Ev2geny Ev2geny May 29, 2022

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.

This is fixed. The spesific documentation is updated and linked to with :class:tempfile.NamedTemporaryFile``

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Doc/whatsnew/3.10.rst Outdated

@zooba zooba May 23, 2022

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.

Needs to move to 3.12.rst, and can be about half the length of what's currently in the NEWS file

Sorry, something went wrong.

Uh oh!

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

All reactions

@Ev2geny Ev2geny May 29, 2022

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.

Implemented. Whats new is moved to 3.12.rst and only 2 lines now

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels May 23, 2022
@bedevere-bot

bedevere-bot commented May 23, 2022

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

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

Ev2geny added 3 commits May 28, 2022 00:58
@Ev2geny
inial update. Tests do not work yet
144db7f
@Ev2geny
delete_on_close flag is added to NamedTemporaryFile functionality
21f7df0
@Ev2geny
possibility for user to delete temporary file is added without conten…
2d7927b
…t manager complaining
@Ev2geny
Ev2geny requested review from 1st1, asvetlov, brandtbucher, iritkatriel, markshannon, methane and tiran as code owners September 18, 2022 22:44
@gvanrossum

gvanrossum commented Sep 18, 2022

Copy link
Copy Markdown
Member

Looks like you made a git mistake. Hopefully someone else can help you (maybe just close this PR and create a new one from a fresh clone of the main branch).

All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum
gvanrossum removed request for a team, 1st1, abalkin, asvetlov, iritkatriel, jaraco, methane, ncoghlan, tiran, vsajip and warsaw September 18, 2022 22:54
@Ev2geny
Revert "Merged with master and resolved merge conflicsts in Doc/whats…
a0a8ae3
…new/3.12.rst and in Lib/tempfile.py"

This reverts commit 07076e8.
@Ev2geny

Ev2geny commented Sep 18, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Looks like you made a git mistake. Hopefully someone else can help you (maybe just close this PR and create a new one from a fresh clone of the main branch).

Yes, this was something with git. I merged master branch into my PR, but it marked lots of files as being changed. I reverted this meanwhile. But I need some guidelines how to move forward.
Since there is a conflict between my PR and a master branch I guess I have to merge or rebase it with master somehow

All reactions

Sorry, something went wrong.

Uh oh!

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

@terryjreedy

terryjreedy commented Sep 19, 2022

Copy link
Copy Markdown
Member

Below is the the fixed up Improved Modules section, with each item in proper alphabetical order. I could not commit it because git requires all conflicts fixed in one merge, and I have no idea how to merge your tempfile change with what someone else did since your patch. Use git log or git blame to find the issue that made the alternative change and what it was meant to do.

replacement text Improved Modules ================

dis

  • Pseudo instruction opcodes (which are used by the compiler but
    do not appear in executable bytecode) are now exposed in the
    :mod:dis module.
    :data:~dis.HAVE_ARGUMENT is still relevant to real opcodes,
    but it is not useful for pseudo instructions. Use the new
    :data:~dis.hasarg collection instead.
    (Contributed by Irit Katriel in :gh:94216.)

os

  • Add :data:os.PIDFD_NONBLOCK to open a file descriptor
    for a process with :func:os.pidfd_open in non-blocking mode.
    (Contributed by Kumar Aditya in :gh:93312.)

pathlib

  • Add :meth:~pathlib.Path.walk for walking the directory trees and generating
    all file or directory names within them, similar to :func:os.walk.
    (Contributed by Stanislav Zmiev in :gh:90385.)

sqlite3

  • Add a :ref:command-line interface <sqlite3-cli>.
    (Contributed by Erlend E. Aasland in :gh:77617.)

tempfile

The :class:tempfile.NamedTemporaryFile class has a new optional parameter
delete_on_close (Contributed by Evgeny Zorin in :gh:58451.)

threading

  • Add :func:threading.settrace_all_threads and
    :func:threading.setprofile_all_threads that allow to set tracing and
    profiling functions in all running threads in addition to the calling one.
    (Contributed by Pablo Galindo in :gh:93503.)

unicodedata

  • The Unicode database has been updated to version 15.0.0. (Contributed by
    Benjamin Peterson in :gh:96734).

Note: a few months ago, you did a force push instead of a simple update merge and push. Force pushes sometimes result in the behavior experienced today and are seldom needed. Unclear exactly what you did today. I recommend you fix conflict in your local clone, then merge upstream/main, then push to fork.

All reactions

Sorry, something went wrong.

Uh oh!

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

@Ev2geny

Ev2geny commented Sep 19, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

@terryjreedy , thanks for this information, but I am afraid I don't think I understand what to do with it and how exactly to move forward.

What I want is to refresh my pull request with whatever is latest in the current main branch. so did merge with the current main, which obviousely introduced a lot of new files, but when I pushed it to github it looks like it made impression, that I tried to modify them, whilst I only transfered them fom the main.
I now have reverted the change, but I still sit with the situation that I want to refresh by PR with whatever is the current main (and yes, a couple of merge conflicts will have to be resolved).

Also, the work on PR is not finished yet, as there were some more recent reviews from @eryksun

All reactions

Sorry, something went wrong.

Uh oh!

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

@JelleZijlstra

JelleZijlstra commented Sep 21, 2022

Copy link
Copy Markdown
Member

There are still conflicts. I'd recommend closing this PR and starting over with the changes you want to make.

All reactions

Sorry, something went wrong.

Uh oh!

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

@Ev2geny

Ev2geny commented Sep 21, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Note: a few months ago, you did a force push instead of a simple update merge and push. Force pushes sometimes result in the behavior experienced today and are seldom needed. Unclear exactly what you did today. I recommend you fix conflict in your local clone, then merge upstream/main, then push to fork.

Just for the record: back then I did rebasing from the master (rather than merge). And then I had to do force push, if I remember correctly

All reactions

Sorry, something went wrong.

Uh oh!

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

@Ev2geny

Ev2geny commented Sep 21, 2022

Copy link
Copy Markdown
Contributor Author

Based on the suggestions of @gvanrossum and @JelleZijlstra I will close this PR and create a new one

All reactions

Sorry, something went wrong.

Uh oh!

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

@Ev2geny Ev2geny mentioned this pull request Sep 22, 2022
gh-58451: Add optional delete_on_close parameter to NamedTemporaryFile #97015
Merged
@Ev2geny

Ev2geny commented Sep 22, 2022

Copy link
Copy Markdown
Contributor Author

I new PR 97015 has been created to replace this one.

ezio-melotti 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.

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
@eryksun eryksun eryksun left review comments
@zooba zooba zooba left review comments
@AA-Turner AA-Turner AA-Turner requested changes
@brettcannon brettcannon Awaiting requested review from brettcannon
@encukou encukou Awaiting requested review from encukou
@ethanfurman ethanfurman Awaiting requested review from ethanfurman
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner
@terryjreedy terryjreedy Awaiting requested review from terryjreedy
@ericvsmith ericvsmith Awaiting requested review from ericvsmith
@rhettinger rhettinger Awaiting requested review from rhettinger
@gpshead gpshead Awaiting requested review from gpshead
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag
@cjw296 cjw296 Awaiting requested review from cjw296
@isidentical isidentical Awaiting requested review from isidentical
@pablogsal pablogsal Awaiting requested review from pablogsal
@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou

Uh oh!

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

Assignees

No one assigned

Labels

awaiting merge

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.

16 participants

@Ev2geny @the-knights-who-say-ni @blais @bozhinov @willingc @MaxwellDupre @AlexWaygood @bedevere-bot @gvanrossum @terryjreedy @JelleZijlstra @merwok @eryksun @zooba @AA-Turner @ezio-melotti
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.