Thanks for the review! I updated the patch, but I made a mistake in the process, and it ended up in a separate thread. Sorry.
This is the message ID of the v2 patch: Message-ID: <[email protected]> > 2026/03/09 2:56、Jan Kiszka <[email protected]>のメール: > > On 08.03.26 11:55, Akihiro Yamazaki wrote: >> rmtree() fails if there is a directory without write permission, >> even if you are the owner. This can be avoided by using >> TemporaryDirectory.cleanup(). >> >> However, there are still cases where it may fail, so in the >> future it may be better to specify ignore_cleanup_errors=True >> (requires Python 3.10 or later). > > Can you provide some examples for those remaining cases? And if we do > that, we would also leave some files and dirs behind, just less, right? On Linux, such remaining cases could involve things like `chattr +i somedir` or the existence of a mount point. However, both require root privileges and seem highly unlikely to occur in practice. Because of this, I found the original comment not very helpful and have removed it. Yes, in this case, if we use `ignore_cleanup_errors=True` we can delete more files, but some files will continue to remain. > Missing signed-off here. Please see CONTRIBUTING.md for its meaning. Oops, sorry, I forgot it. > >> --- >> kas/libcmds.py | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/kas/libcmds.py b/kas/libcmds.py >> index 3a651bf..bfe320c 100644 >> --- a/kas/libcmds.py >> +++ b/kas/libcmds.py >> @@ -191,11 +191,12 @@ class SetupHome(Command): >> >> def __init__(self): >> super().__init__() >> + self.tmpdir = None >> self.tmpdirname = None >> >> def __del__(self): >> - if self.tmpdirname: >> - shutil.rmtree(self.tmpdirname) >> + if self.tmpdir: >> + self.tmpdir.cleanup() >> >> def __str__(self): >> return 'setup_home' >> @@ -382,8 +383,9 @@ class SetupHome(Command): >> managed_env = get_context().managed_env >> if managed_env: >> logging.info(f'Running on {managed_env}') >> - if not self.tmpdirname: >> - self.tmpdirname = tempfile.mkdtemp() >> + if not self.tmpdir: >> + self.tmpdir = tempfile.TemporaryDirectory() >> + self.tmpdirname = self.tmpdir.name > > How about a global conversion of self.tmpdirname -> self.tmpdir.name? > Would allow us to drop this extra variable. I was initially hesitant to replace self.tmpdirname due to the size of the diff, but I have gone ahead and made the change to use self.tmpdir.name as you suggested, as it looks clean. >> def_umask = os.umask(0o077) >> self._setup_netrc() >> self._setup_npmrc() > > Thanks for addressing this! > > Jan > -- Yamazaki Akihiro [email protected] MODE, Inc. -- You received this message because you are subscribed to the Google Groups "kas-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion visit https://groups.google.com/d/msgid/kas-devel/DBA32874-A7B0-4F0A-8095-DF9C6005D188%40tinkermode.com.
