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

Clean up cask audit tmpdir after use #17358

Merged
merged 1 commit into from
May 24, 2024

Conversation

samford
Copy link
Member

@samford samford commented May 24, 2024

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Cask::Audit#extract_artifacts is used in the #audit_signing and #cask_plist_min_os methods to create a directory in /tmp and extract cask artifacts without duplicating the work if it's already done. However, due to how this is set up, tmpdir isn't removed afterward and the extracted artifacts will take up disk space until the tmp directory is cleaned up. As a result, running brew audit --strict --online locally can chew through disk space and it may not be clear to the user where their free space has gone.

This adds a finalizer method to Cask::Audit to remove the created @tmpdir (if any) once it's no longer needed. There may be a better way of addressing the issue but this works for now without having to restructure how these audits work.


As discussed on Slack, it probably makes sense for brew cleanup to also clean up tmp directories that brew has created. From a quick glance at our use of #mktmpdir, we would probably have to institute a common prefix (e.g., using homebrew- or homebrew-brew- before existing prefixes) to be able to reasonably identify brew-created tmp directories (without manually maintaining a list of prefixes in use). There may be a better way of going about it but that was my initial thought.

That said, it may not be necessary if everything's working correctly. With this patch in place, I've only ended up with a lingering homebrew-unpack tmp directory on maybe one or two occasions but I haven't figured out why that happened (my best guess is that I may have prematurely interrupted a command using ^C).

`Cask::Audit#extract_artifacts` is used in the `#audit_signing` and
`#cask_plist_min_os` methods to create a directory in `/tmp` and
extract cask artifacts without duplicating the work if it's already
done. However, due to how this is set up, `tmpdir` isn't removed
afterward and the extracted artifacts will take up disk space until
the `tmp` directory is cleaned up. As a result, running
`brew audit --strict --online` locally can chew through disk space
and it may not be clear to the user where their free space has gone.

This adds a finalizer method to `Cask::Audit` to remove the created
`@tmpdir` (if any) once it's no longer needed. There may be a better
way of addressing the issue but this works for now without having to
restructure how these audits work.
@MikeMcQuaid
Copy link
Member

As discussed on Slack, it probably makes sense for brew cleanup to also clean up tmp directories that brew has created. From a quick glance at our use of #mktmpdir, we would probably have to institute a common prefix (e.g., using homebrew- or homebrew-brew- before existing prefixes) to be able to reasonably identify brew-created tmp directories (without manually maintaining a list of prefixes in use). There may be a better way of going about it but that was my initial thought.

This is a good idea 👍🏻

(my best guess is that I may have prematurely interrupted a command using ^C).

I think we should still endeavour to have brew cleanup handle these.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for PR! Approach seems fine for now to fix the bug but I think ideally this would have the tmpdir usage be all within one block so cleanup happens as typical at the end of the block.

@MikeMcQuaid MikeMcQuaid merged commit 86674b4 into Homebrew:master May 24, 2024
25 checks passed
@samford samford deleted the cask-audit-tmpdir-cleanup branch May 24, 2024 14:59
@samford samford mentioned this pull request May 27, 2024
7 tasks
@github-actions github-actions bot added the outdated PR was locked due to age label Jun 24, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants