Skip to content

Convert quarantine script to use FFI rather than Swift#22377

Open
Bo98 wants to merge 3 commits into
mainfrom
quarantine-ffi
Open

Convert quarantine script to use FFI rather than Swift#22377
Bo98 wants to merge 3 commits into
mainfrom
quarantine-ffi

Conversation

@Bo98
Copy link
Copy Markdown
Member

@Bo98 Bo98 commented May 22, 2026

Had been toying around with this idea for a year or so now, finally managed to clean it up enough to hopefully be shippable.

Swift is a great language to compile programs under but isn't very reliable as a psuedo-scripting language (which it's really not meant to be) like we were trying to do. It has a number of problems:

  1. It required a check_quarantine_support that reports 7(!) different reasons that quarantine might not work on a system - and 6 of them are Swift related
  2. It doesn't even work in Homebrew/cask's macos-14 CI! https://github.com/Homebrew/homebrew-cask/actions/runs/25932611701/job/76231319882#step:11:205. This could easily happen to an end user machine, even on newer macOS.
  3. It's not always obvious when it fails (hence why nobody noticed cask CI)
  4. There's been too many gotchas over the years (can't use stderr: cask: fix trash.swift under Xcode 16 #17514, arg parsing doesn't work on some legacy systems, breaks with some CLT updates).

End goal is to delete check_quarantine_support entirely and make it always enabled on macOS. I still need to port copy-xattrs.swift first before that happens, so that change is not contained in this PR. I'd like to see if this approach proves stable and reliable for the current set of users before expanding it.


  • 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? Performance claims (e.g. "this is faster") must include Hyperfine benchmarks.
  • Have you written new tests (excluding integration tests) for your changes? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) with your changes locally?

  • AI was used to generate or assist with generating this PR. Please specify below how you used AI to help you, and what steps you have taken to manually verify the changes. Non-maintainers may only have one AI-assisted/generated PR open at a time.

Copilot AI review requested due to automatic review settings May 22, 2026 03:57
@Bo98 Bo98 changed the title Quarantine ffi Convert quarantine script to use FFI rather than Swift May 22, 2026
@Bo98 Bo98 force-pushed the quarantine-ffi branch from 240f8ee to cc6a90b Compare May 22, 2026 04:02
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR replaces the Swift-based Cask quarantine helper with a Ruby/Fiddle (FFI) implementation on macOS, and factors out a reusable macOS FFI wrapper (also used for shared-cache dylib detection).

Changes:

  • Add OS::Mac::FFI wrappers for libSystem/CoreFoundation/LaunchServices via Fiddle.
  • Switch Cask quarantine application from invoking quarantine.swift to setting quarantine properties via CoreFoundation/LaunchServices FFI.
  • Refactor macOS LinkageChecker shared-cache detection to use the new FFI wrapper and simplify the base checker logic.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
Library/Homebrew/os/mac/ffi.rb New macOS Fiddle-based wrappers for dyld/CoreFoundation/LaunchServices APIs.
Library/Homebrew/linkage_checker.rb Simplifies “system dylib in shared cache” detection path.
Library/Homebrew/extend/os/mac/linkage_checker.rb Uses the new FFI wrapper for _dyld_shared_cache_contains_path and removes old inline Fiddle setup.
Library/Homebrew/cask/quarantine.rb Replaces Swift quarantine execution with direct CoreFoundation/LaunchServices FFI calls.
Library/Homebrew/cask/utils/quarantine.swift Removes the Swift quarantine script previously used by quarantine support checks and quarantining.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread Library/Homebrew/cask/quarantine.rb
Comment thread Library/Homebrew/os/mac/ffi.rb Outdated
Comment thread Library/Homebrew/os/mac/ffi.rb Outdated
Comment thread Library/Homebrew/cask/quarantine.rb
@Bo98 Bo98 force-pushed the quarantine-ffi branch 4 times, most recently from 33bed90 to bc54be7 Compare May 22, 2026 05:17
Copy link
Copy Markdown
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.

This is interesting, thanks for the PR @Bo98. Things I think we need before merge:

  • a lot more comments, the implementation is much harder to follow than the Swift one, and I don't even "know" Swift
  • a lot more tests, particularly unit tests etc. for the ffi.rb logic. In 2026, it should be very straightforward to get Claude or Codex or similar to do >90% of the work here.

Optional but recommended:

  • I think it would make sense to also port the other script you mention and remove all Swift usage. Again, I think given you've got the foundations here, AI should make it fairly straightforward to follow the same approach and I think seeing this work with N > 1 would make the approach easier to review

odebug "Quarantining #{download_path}"

raise "unexpected nil swift" unless swift
require "os/mac/ffi"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Probably shouldn't import os/mac stuff inside cask/quarantine now casks support macOS and Linux.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Existing code already was entirely macOS specific. I suppose because Quarantine.available? is only ever true on macOS.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeh, still feels a bit weird and perhaps a chance for some refactoring? Don't feel super strongly though.

@MikeMcQuaid
Copy link
Copy Markdown
Member

I'd like to see if this approach proves stable and reliable for the current set of users before expanding it.

May also want to consider opt-in/opt-out given that so if the approach does not seem stable/reliable we don't have to tag a new release to revert this for individual users.

@Bo98 Bo98 force-pushed the quarantine-ffi branch from bc54be7 to 6e98d4d Compare May 22, 2026 15:38
@Bo98
Copy link
Copy Markdown
Member Author

Bo98 commented May 22, 2026

a lot more comments, the implementation is much harder to follow than the Swift one, and I don't even "know" Swift

I agree. For clarity, are you referring more to quarantine.rb bits, ffi.rb bits or both?

The general goal is that:

  • quarantine.rb would roughly look like how quarantine.swift looked before (though using CoreFoundation rather than Foundation so some function names will be slightly different and have more CFStringRef wrapping).
  • ffi.rb is a thin 1:1 mapping where one Ruby function is mapped to one C function (or constant)

In fact, I should probably be mentioning that mapping a bit more so I've added comments to ffi.rb to do that.

a lot more tests, particularly unit tests etc. for the ffi.rb logic.

I guess challenge here is that some functions need the result of another one so might get very close to just copying quarantine.rb code. I know you didn't mention it but I think quarantine.rb could do with some unit tests (partly why I left that one Copilot review above unresolved), basically checking to see if xattr -p com.apple.quarantine #{executable} matches what is expected.

I've gone ahead and added unit tests for quarantine.rb.

I'd like to see if this approach proves stable and reliable for the current set of users before expanding it.

May also want to consider opt-in/opt-out given that so if the approach does not seem stable/reliable we don't have to tag a new release to revert this for individual users.

I think I was being overly cautious in that sentence. When I initially thought about doing this a while back, we had never used FFI. Since then, we do use FFI for all users in _dyld_shared_cache_contains_path and that has had no issues at all.

@Bo98 Bo98 force-pushed the quarantine-ffi branch from 6e98d4d to 5344ec8 Compare May 22, 2026 15:41
@MikeMcQuaid
Copy link
Copy Markdown
Member

I agree. For clarity, are you referring more to quarantine.rb bits, ffi.rb bits or both?

both ideally

I know you didn't mention it but I think quarantine.rb could do with some unit tests (partly why I left that one Copilot review above unresolved), basically checking to see if xattr -p com.apple.quarantine #{executable} matches what is expected.

even if it's just the subset that don't that'd be good or e.g. chaining multiple calls together

When I initially thought about doing this a while back, we had never used FFI. Since then, we do use FFI for all users in _dyld_shared_cache_contains_path and that has had no issues at all.

Ok, good. In that case I think it's probably worth porting both Swift scripts at once.

Copy link
Copy Markdown
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.

Looking better! More tests ideal but this is a big improvement.

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.

3 participants