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

[stdlib] tempfile and tempdir #2352

Closed

Conversation

artemiogr97
Copy link
Contributor

Implementation for #2018

@artemiogr97 artemiogr97 changed the title Tempfile-and-tempdir [stdlib] tempfile and tempdir Apr 20, 2024
@artemiogr97
Copy link
Contributor Author

artemiogr97 commented Apr 20, 2024

I opened the pull to have some feedback, some points to discuss:

  • Are the read methods useful in NamedTemporaryFile?
    • personally I don't know in which case I would create a temporary file and read from it at the same time
  • should I implement what I need in the os module directly in this pull ?
    • TemporaryDirectory is not implemented yet because os.mkdir and os.rmdir don't exist, I guess shutil.rmtree will be necessary too (implemented in [stdlib] Implement mkdir and rmdir #2430 and [stdlib] Implementation of rmtree #2439)
    • For NamedTemporaryFile there is a slight difference with respect to the python implementation, the name attribute could end up being relative but the functions os.abspath, and os.normpath still don't exist
      • also how different platforms are going to be handled in mojo, in python there are dedicated modules posix and ntpath, I guess the same should be done in mojo?

Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>
Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>
@artemiogr97 artemiogr97 force-pushed the tempfile-and-tempdir branch 2 times, most recently from a0b40cc to a8d1ea3 Compare April 28, 2024 13:05
Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>
Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>
Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>
@artemiogr97
Copy link
Contributor Author

#2439 must be merged first

@artemiogr97 artemiogr97 marked this pull request as ready for review May 4, 2024 09:32
@artemiogr97 artemiogr97 requested a review from a team as a code owner May 4, 2024 09:33
@ematejska ematejska added the mojo-repo Tag all issues with this label label May 6, 2024
@JoeLoser
Copy link
Collaborator

#2439 must be merged first

Sorry for the delay here. Is it possible to decouple this PR from #2439? I think temporary file things would be useful even outside of the rmtree functionality, so I'd rather not couple them.

FYI @laszlokindrat @Dan13llljws @ConnorGray as we were talking about temp files today.

@laszlokindrat laszlokindrat self-assigned this May 16, 2024
@laszlokindrat
Copy link
Contributor

laszlokindrat commented May 16, 2024

Sorry for the delay here. Is it possible to decouple this PR from #2439? I think temporary file things would be useful even outside of the rmtree functionality, so I'd rather not couple them.

I completely agree. @artemiogr97 Can you please rebase this on the latest nightly, and maybe start splitting it up? Also one concrete suggestion: could you please start with the low level tempfile APIs, i.e. gettempdir and mkdtemp, and maybe mkstemp, and then we can build the rest on it.

This super cool btw, we're very happy that you are pushing this forward!

@artemiogr97
Copy link
Contributor Author

artemiogr97 commented May 18, 2024

could you please start with the low level tempfile APIs, i.e. gettempdir and mkdtemp, and maybe mkstemp, and then we can build the rest on it.

@laszlokindrat I started working on this, I'm having some issues with mkstemp, in python it returns a tuple with the file object and the name, I'm not sure how to do this in mojo, the problem is that when I do the following I have an error:

tmp_file_and_name = msktemp()
tmp_file = tmp_file_and_name[0]  # error here: 'FileHandle' is not copyable because it has no '__copyinit__'
file_name = tmp_file_and_name[1]

I tried doing tmp_file = tmp_file_and_name.get[0, FileHandle]() but then I have a double free at runtime.
I guess that the best option is to return only the file handle for now? but we wont have access to the file name

Edit: I just realized that returning only the file handle is not useful, there would not be a way to delete the file after using it

@artemiogr97
Copy link
Contributor Author

I opened a new PR (#2742) that implements gettempdir and mkdtemp, and a second PR ( #2743) for TemporaryDirectory using mkdtemp

@artemiogr97
Copy link
Contributor Author

@JoeLoser, @laszlokindrat I opened another PR #2762 for NamedTemporaryFile seems so be easier to implement than mkstemp for now, I opened it as a draft since I'm having some issues, I'll leave you all the details in that PR.

Also I guess I can close this PR and also #2439 since everything is now splitted in #2742, #2743 and #2762

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mojo-repo Tag all issues with this label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants