[stdlib] implement mkdtemp #2742

artemiogr97 · 2024-05-18T19:02:08Z

No description provided.

artemiogr97 · 2024-05-18T19:37:10Z

Part of the implementation for #2018

laszlokindrat

Very nice, thanks for pushing this forward! This is shaping up nicely, I would just like to see some tests for gettempdir before we bring it in.

stdlib/src/tempfile/tempfile.mojo

stdlib/test/tempfile/test_tempfile.mojo

stdlib/src/tempfile/tempfile.mojo

artemiogr97 · 2024-05-23T20:07:14Z

stdlib/test/tempfile/test_tempfile.mojo

+        )
+
+    # test gettempdir falls back to TMP
+    with _TempEnvForTest(


I guess that this test is going to fail in the internal CI, similar to what happens here:

mojo/stdlib/test/os/test_mkdir_and_rmdir.mojo

Lines 84 to 94 in 011bf40

mkdir(my_dir_path, 0o111)

# TODO: This test is failing on Graviton internally in CI, revisit.

# with assert_raises(contains="Permission denied"):

# var file = open(file_name, "w")

# file.close()

# if exists(file_name):

# remove(file_name)

if exists(my_dir_path):

rmdir(my_dir_path)

artemiogr97 · 2024-05-23T20:08:55Z

@laszlokindrat I added some tests for gettempdir, but maybe 1 test is not going to work, see my comment above

laszlokindrat

Nice, thanks! This looks great to me, I just have a few more requests for change, nothing major.

stdlib/src/tempfile/tempfile.mojo

laszlokindrat · 2024-05-26T13:11:31Z

stdlib/src/tempfile/tempfile.mojo

+    constrained[not sys.os_is_windows(), "windows not supported yet"]()
+
+    var dirlist = List[String]()
+    var possible_env_vars = List("TMPDIR", "TEMP", "TMP")


Could this be an alias?

changing this to be an alias causes an issue at runtime:

+ mojo /home/temo/repos/mojo/stdlib/test/tempfile/test_tempfile.mojo <unknown>:0: error: failed to legalize operation 'kgen.param.constant' that was explicitly marked illegal <unknown>:0: note: see current operation: %2 = "kgen.param.constant"() {value = #interp.store_to_mem<#kgen.struct<#interp.memref<[(memory_blob_bc35f10c536d6691ed3bb9bfa830df1b0bc51395ed96fff37be4aab479df5484, heap, []), (memory_blob_79210cf7423b267ddb5a7bc898de233a883230728f74e5fced11b22adb5eb763, stack, [(0, 0, 0)]), (memory_blob_9c18dd61ff20f539c03848a0ac560e46cc48cc8629755a8236a7c3b7ce7f2026, stack, []), (memory_blob_0e63b401645577494c63bffad4c5e392342e2b004bfd3283a98daf47d391b60d, stack, []), (memory_blob_2bc6fefda0788d1117810a96ee52f69e7ba6c9e594aa2c39c37435a0ff3bcb51, stack, []), (static_string_14f8defe8295f90f25152e66dd3608c11b5b242e9f917c5c31bcabb7c16dd1b7, const_global, []), (static_string_e810509f0bd1009a376d10738b852aa41d0f7edcfe8210f45e95bd7027a9b056, const_global, []), (static_string_b6801ff93b1ce1c9aa784dc10cd72a98d154d8edeb17ea9b1d48d460119c54cf, const_global, []), (memory_blob_bbebd66f64306b5de8c1e7a61f72f2f9b60099f333e5fbe9ab634e6a1e7b7de8, persistent, [(0, 2, 0), (8, 3, 0), (16, 4, 0)])], 0, 0>, 3, 3> : !kgen.struct<(pointer<none>, index, index) memoryOnly>> : !kgen.pointer<struct<(pointer<none>, index, index) memoryOnly>>} : () -> !kgen.pointer<struct<(pointer<none>, index, index) memoryOnly>> mojo: error: Failed to materialize symbols: { (exec, { KGEN_EE_JIT_GlobalDestructor, main, KGEN_EE_JIT_GlobalConstructor }) } (from the layer: failed to lower module to LLVM IR for archive compilation)

I think this is the same regression that broke @gabrieldemarmiesse work on SSO from yesterday.

@laszlokindrat want me to just revert rather than waiting since I think we have two distinct breakages out in the wild in flight now?

As we discussed, @JoeLoser will try to fix this regression by reverting. @artemiogr97 I will just add a TODO here to improve this.

stdlib/src/tempfile/tempfile.mojo

laszlokindrat · 2024-05-26T13:32:48Z

stdlib/test/tempfile/test_tempfile.mojo

+    assert_false(exists(dir_name), "Failed to delete temporary directory")
+
+
+struct _TempEnvForTest:


Nice! Followup idea: we could potentially move this to the os module, or at least to test utils. I always found temporary environment settings useful.

I just made _TempEnvForTest more generic (using dict instead of hard-coded values), I can move it to test_utils if you want

Nice, thanks! Let's keep it as is for now; once it lands, you can circle back to it if you want to, it would be highly appreciated! :)

stdlib/test/tempfile/test_tempfile.mojo

Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>

laszlokindrat · 2024-05-26T18:38:00Z

stdlib/test/tempfile/test_tempfile.mojo

+    fn __init__(
+        inout self,
+        vars_to_set: Dict[String, String],
+        clean_up_function: fn () raises -> None,
+    ):


non-blocking: This is fine for now, since it's just a local testing utility. If we move this to os or testing, we should add an overload that takes variadic keyword arguments. We should also consider, whether clean_up_function should be a parameter.

laszlokindrat

This is awesome, thank you so much for working on this! It might take a bit on my end to land this, but I hope it will make it in the next nightly.

laszlokindrat · 2024-05-26T18:40:20Z

!sync

laszlokindrat · 2024-05-27T08:58:39Z

I added an empty tempfile module internally to help landing this. ~~Could you please rebase, and also add/modify the appropriate cmake files?~~ Nevermind, I have to do that it seems.

modularbot · 2024-05-27T21:09:24Z

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

modularbot · 2024-05-28T05:50:24Z

Landed in fd697f0! Thank you for your contribution 🎉

[External] [stdlib] Implement `tempfile.{mkdtemp,gettempdir}` A new `tempfile` module is added with these two functions. Currently limited to MacOS and Linux. ORIGINAL_AUTHOR=artemiogr97 <57588855+artemiogr97@users.noreply.github.com> PUBLIC_PR_LINK=#2742 --------- Co-authored-by: artemiogr97 <57588855+artemiogr97@users.noreply.github.com> Closes #2742 MODULAR_ORIG_COMMIT_REV_ID: 943428af5d911d2f82d723f54b4029480d68ec5c

artemiogr97 requested a review from a team as a code owner May 18, 2024 19:02

This was referenced May 18, 2024

[stdlib] Temporary directory #2743

Open

[stdlib] tempfile and tempdir #2352

Closed

artemiogr97 force-pushed the mkdtemp branch from bd523e7 to 87af999 Compare May 18, 2024 19:32

artemiogr97 requested a review from a team as a code owner May 18, 2024 19:32

laszlokindrat self-assigned this May 20, 2024

artemiogr97 mentioned this pull request May 20, 2024

[stdlib] NamedTemporaryFile #2762

Draft

laszlokindrat reviewed May 22, 2024

View reviewed changes

artemiogr97 force-pushed the mkdtemp branch 2 times, most recently from ef2b4c0 to 7a645ad Compare May 23, 2024 20:02

artemiogr97 commented May 23, 2024

View reviewed changes

laszlokindrat reviewed May 26, 2024

View reviewed changes

artemiogr97 added 2 commits May 26, 2024 18:30

[stdlib] implement mkdtemp

219e74b

Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>

[stdlib] minor changes in mkdtemp and add tests for gettempdir

af7ac01

Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>

artemiogr97 force-pushed the mkdtemp branch from 7a645ad to af7ac01 Compare May 26, 2024 16:30

artemiogr97 added 3 commits May 26, 2024 19:05

[stdlib] minor simplifications and some explanations added

944511c

Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>

[stdlib] use unitilialized list in _get_random_name

cc57502

Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>

[stdlib] make _TempEnvForTest more generic

6ea6139

Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>

laszlokindrat reviewed May 26, 2024

View reviewed changes

laszlokindrat approved these changes May 26, 2024

View reviewed changes

modularbot added the imported-internally Signals that a given pull request has been imported internally. label May 26, 2024

artemiogr97 mentioned this pull request May 26, 2024

[BUG] using walrus operator needs variable declaration #2841

Open

modularbot added merged-internally Indicates that this pull request has been merged internally merged-externally Merged externally in public mojo repo labels May 27, 2024

modularbot closed this May 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stdlib] implement mkdtemp #2742

[stdlib] implement mkdtemp #2742

artemiogr97 commented May 18, 2024

artemiogr97 commented May 18, 2024

laszlokindrat left a comment

artemiogr97 May 23, 2024

artemiogr97 commented May 23, 2024

laszlokindrat left a comment

laszlokindrat May 26, 2024

artemiogr97 May 26, 2024

JoeLoser May 26, 2024

JoeLoser May 26, 2024

laszlokindrat May 26, 2024

laszlokindrat May 26, 2024

artemiogr97 May 26, 2024

laszlokindrat May 26, 2024

laszlokindrat May 26, 2024

laszlokindrat left a comment

laszlokindrat commented May 26, 2024

laszlokindrat commented May 27, 2024 •

edited

modularbot commented May 27, 2024

modularbot commented May 28, 2024

	mkdir(my_dir_path, 0o111)

	# TODO: This test is failing on Graviton internally in CI, revisit.
	# with assert_raises(contains="Permission denied"):
	# var file = open(file_name, "w")
	# file.close()
	# if exists(file_name):
	# remove(file_name)

	if exists(my_dir_path):
	rmdir(my_dir_path)

		assert_false(exists(dir_name), "Failed to delete temporary directory")


		struct _TempEnvForTest:

[stdlib] implement mkdtemp #2742

[stdlib] implement mkdtemp #2742

Conversation

artemiogr97 commented May 18, 2024

artemiogr97 commented May 18, 2024

laszlokindrat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artemiogr97 commented May 23, 2024

laszlokindrat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laszlokindrat left a comment

Choose a reason for hiding this comment

laszlokindrat commented May 26, 2024

laszlokindrat commented May 27, 2024 • edited

modularbot commented May 27, 2024

modularbot commented May 28, 2024

laszlokindrat commented May 27, 2024 •

edited