Skip to content

Releasing temporary files#2893

Closed
oscarlevin wants to merge 2 commits into
PreTeXtBook:masterfrom
oscarlevin:tmp-files
Closed

Releasing temporary files#2893
oscarlevin wants to merge 2 commits into
PreTeXtBook:masterfrom
oscarlevin:tmp-files

Conversation

@oscarlevin

Copy link
Copy Markdown
Member

Two commits:

  1. Wrap the releasing in a try/catch to avoid errors when Windows is being difficult. I did also put the resetting of the list of temporary directories outside the for loop. Don't think it actually matters, but looks better to me.
  2. I added a parameter in the function that allows the CLI to always ask for the temporary directories to be released. This is something @bnmnetp has wanted, since he uses debug-level logging when building in runestone, but doesn't want all the temporary directories to clutter up his build server. Shouldn't make any difference when using the core pretext script.

Of course, it's up to you if you want to keep both or just the first one.

rbeezer added a commit that referenced this pull request Jun 7, 2026
@rbeezer

rbeezer commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

Thanks very much, @oscarlevin. Good catch on the reset of __temps to the empty list inside the loop! That was definitely not right. Claude and I both had things to say, so I just built a follow-on commit and let Claude write an "action report," coming next. Merged as-is.

@rbeezer

rbeezer commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

Thanks @oscarlevin. I added a small follow-on commit from a review pass — summary below, as review item → disposition:

1. Required any_log_level breaks existing callers.
Item: the new parameter has no default, so any caller of release_temporary_directories() that doesn't pass it (e.g. vendored copies) breaks; a default of False would keep them working.
Decision: Left as-is, on purpose — we want consumers to notice and update. 😉

2. Wrong log level on successful removal.
Item: log.warning("Removed temporary directory …") fires on every success — wrong level, and redundant with the preceding log.info.
Change: dropped to log.debug.

3. Stale comment.
Item: # conservatively, raise exception on errors no longer matched, since the new try/except catches that raise.
Change: reworded to "let a removal failure raise, so it is caught and reported below".

4. Bare except, and the failure didn't say why.
Item: except: also swallows KeyboardInterrupt/SystemExit, and the warning didn't include the cause.
Change: now except Exception as e:, and the message reports it: … (and maybe some others): {}".format(td, str(e)).

5. __temps not emptied on partial failure.
Item: if removal raises partway, the __temps = [] after the loop is skipped, so the global keeps both already-removed and remaining dirs — which a long-running host process (e.g. Runestone) could accumulate across builds.
Change: moved the reset into a finally, so __temps is always emptied even when a removal raises.

6. Double blank line after the docstring — removed.

Claude Opus 4.8, acting as a review assistant for Rob Beezer

@rbeezer rbeezer closed this Jun 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants