Skip to content

Supports in repository patch files#22144

Open
rexmhall09 wants to merge 3 commits intoHomebrew:mainfrom
rexmhall09:main
Open

Supports in repository patch files#22144
rexmhall09 wants to merge 3 commits intoHomebrew:mainfrom
rexmhall09:main

Conversation

@rexmhall09
Copy link
Copy Markdown
Contributor

@rexmhall09 rexmhall09 commented May 5, 2026

What does this PR do?

This recreates the local patch file support from closed PR #22136 on top of current main.

It allows formula patches to reference patch files stored in the repository, while keeping the implementation
scoped to the patch-file feature itself. I intentionally left the earlier class-extraction refactor out of this PR
to keep the diff focused and easier to review.

Why is this useful?

This makes larger or reusable patches easier to maintain than embedding them directly in formula files, while
preserving existing patch behavior and test coverage.


  • 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 must include Hyperfine benchmarks.
  • Have you written new tests, excluding integration tests, for your changes?
  • Have you successfully run brew lgtm with your changes locally? (not really needed)

  • AI was used to generate or assist with generating this PR.

I used GPT 5.5 via the oh-my-pi coding agent to recreate the local patch file feature from the old closed PR #22136 on top
of current main, while accounting for maintainer feedback. I manually reviewed the resulting diff, kept the
class-extraction refactor out of this PR, and verified the feature with targeted tests.

Verified with:

  • ./bin/brew tests --only=patch
  • ./bin/brew tests --only=patching
  • ./bin/brew tests --only=api/formula
  • ./bin/brew typecheck
  • ./bin/brew style --fix --changed

Introduce support for local patch files stored in formula repositories. Adds a LocalPatch class (Library/Homebrew/local_patch.rb) and a Resource#file DSL to declare patch files; the Patch factory now produces LocalPatch when a resource.file is set and validates incompatible options (url, sha256, directory, apply). API/formula.rb gained source_download_path (with path sanitization) and now fetches local patch files from HOMEBREW_CACHE_API_SOURCE when loading a formula from the API. RuboCop patch checks were adjusted to iterate multiple url calls. Numerous tests were added/updated to cover local patch behavior and path validation.
Copilot AI review requested due to automatic review settings May 5, 2026 21:35
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 adds support for formula patch do blocks to reference patch files stored within the same formula repository (e.g., tap-local patches/*.diff), rather than requiring inline patches or external patch URLs. It introduces a new file DSL entry for patch resources and ensures local patch files can be resolved both for locally loaded formulae and for formulae loaded via the API source cache.

Changes:

  • Add a file patch DSL that produces a new LocalPatch type, applied like an embedded patch but sourced from a repository-relative file path.
  • Teach API source formula loading to also fetch referenced local patch files into the API source cache.
  • Update FormulaAudit/RuboCop patch auditing and expand test coverage for local patch file behavior and edge cases.

Reviewed changes

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

Show a summary per file
File Description
Library/Homebrew/patch.rb Creates LocalPatch when a patch block uses file, and rejects incompatible DSL combinations.
Library/Homebrew/local_patch.rb Implements LocalPatch to read patch contents from a repository-relative path with realpath-based escape protection.
Library/Homebrew/resource.rb Extends Resource::Patch with a file DSL method and basic path validation.
Library/Homebrew/api/formula.rb Adds source_download_path helper and fetches local patch files when loading formula source via API cache.
Library/Homebrew/rubocops/patches.rb Avoids assuming every patch do block has a url, so local file patches don’t trigger errors/offenses.
Library/Homebrew/test/patch_spec.rb Adds unit coverage for LocalPatch creation and DSL validation errors.
Library/Homebrew/test/patching_spec.rb Adds system-level coverage for applying local patch files (path-loaded formulae, taps, strip levels, missing files, symlink escape).
Library/Homebrew/test/api/formula_spec.rb Verifies local patch files are fetched/read correctly when formula source is loaded from the API source cache.
Library/Homebrew/test/rubocops/patches_spec.rb Ensures RuboCop patch auditing reports no offenses for local file patches.

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

Comment thread Library/Homebrew/local_patch.rb Outdated
Comment thread Library/Homebrew/resource.rb Outdated
Comment thread Library/Homebrew/local_patch.rb
Tighten validation for local patch files and make API cache lookup robust. LocalPatch now raises if the referenced patch is not a file and falls back to HOMEBREW_CACHE/"api-source" when Homebrew::API::HOMEBREW_CACHE_API_SOURCE isn't defined. Resource#file validation was strengthened to reject blank strings, "."/"..", trailing slashes, and absolute paths. Added/updated tests to cover blank/current/directory paths and the new file-existence error.
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.

Thanks @rexmhall09! looking good so far. a few comments.

Comment thread Library/Homebrew/rubocops/patches.rb Outdated
Comment on lines +491 to +496
invalid_path = path_string.blank? ||
path_string.end_with?("/") ||
path.absolute? ||
%w[. ..].include?(path.to_s) ||
path.to_s.start_with?("../")
raise ArgumentError, "Patch file must be a relative path within the repository." if invalid_path
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.

This logic is identical in two places. Would be nice to share, perhaps in a class method in local_patch.rb

Made a line a bit cleaner.

Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>
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