Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Sep 9, 2025

This PR fixes an issue where Homebrew-created files (such as service definition files) were not being properly relocated when using the new /usr/local relocation logic on Intel macOS systems. This was causing bottle creation failures for formulae that generate service files containing generic paths like /usr/local/bin.

Problem

The current keg relocation logic uses selective path replacement when HOMEBREW_PREFIX=/usr/local to avoid overzealous replacement of system paths. However, this meant that Homebrew-generated service files containing paths like:

ExecStart=/usr/local/bin/some-command
Environment="PATH=/usr/local/sbin:/usr/sbin:/usr/bin:/bin:/usr/local/bin"
WorkingDirectory=/usr/local/var/something

Were not getting their /usr/local paths replaced with @@HOMEBREW_PREFIX@@ placeholders, making bottles non-relocatable.

Solution

The fix introduces minimal logic to identify Homebrew-created files and apply full path relocation to them:

  1. File Detection: Added homebrew_created_file?() method that identifies service files (.plist, .service, .timer) with names starting with "homebrew."

  2. Inline Relocation Logic: Modified existing replace_text_in_files() method to use full prefix replacement for Homebrew-created files when using selective relocation, avoiding code duplication

  3. Conditional Processing: Only applies full relocation to Homebrew files when new_usr_local_relocation? is true, maintaining selective relocation for regular files

Result

Service files now get properly relocated:

ExecStart=@@HOMEBREW_PREFIX@@/bin/some-command
Environment="PATH=@@HOMEBREW_PREFIX@@/sbin:/usr/sbin:/usr/bin:/bin:@@HOMEBREW_PREFIX@@/bin"
WorkingDirectory=@@HOMEBREW_PREFIX@@/var/something

This ensures bottles can be successfully created and relocated between different Homebrew installations without breaking service functionality.

The implementation follows DRY and YAGNI principles with minimal code changes (21 lines added) by reusing existing relocation logic rather than duplicating it.

Fixes #20627.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@Copilot Copilot AI changed the title [WIP] Keg relocation should always work for homebrew created files Fix keg relocation for Homebrew-created files to ensure proper bottling Sep 9, 2025
Copilot finished work on behalf of MikeMcQuaid September 9, 2025 07:52
@Copilot Copilot AI requested a review from MikeMcQuaid September 9, 2025 07:52
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.

@copilot The approach is sensible but you've added far, far more code than is necessary. Simplify things: DRY, YAGNI, etc. Try to make the bare minimum number of changes in order to achieve the result. The fewer lines in the diff: the better.

Copy link
Contributor Author

Copilot AI commented Sep 9, 2025

@copilot The approach is sensible but you've added far, far more code than is necessary. Simplify things: DRY, YAGNI, etc. Try to make the bare minimum number of changes in order to achieve the result...

Simplified the approach to use DRY and YAGNI principles. Reduced the diff from 198 lines to 44 lines by:

  1. Reusing existing relocation logic instead of duplicating it
  2. Adding minimal homebrew_created_file? helper method
  3. Modifying existing replace_text_in_files method to handle homebrew files inline
  4. Moving tests to keg_spec.rb instead of separate file

Commit: ac2d888

Copilot finished work on behalf of MikeMcQuaid September 9, 2025 12:12
@Copilot Copilot AI requested a review from MikeMcQuaid September 9, 2025 12:12
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.

@copilot Still far too much new logic here. Trim it down a lot. Throw away all existing logic and start again.

Copy link
Contributor Author

Copilot AI commented Sep 9, 2025

@copilot Still far too much new logic here. Trim it down a lot. Throw away all existing logic and start again.

Threw away the existing logic and implemented a much simpler approach. Reduced from 198 lines to just 21 lines of changes by adding a minimal helper method and modifying the existing replace_text_in_files method inline. Commit: 9be4ca2

Copilot finished work on behalf of MikeMcQuaid September 9, 2025 12:36
@Copilot Copilot AI requested a review from MikeMcQuaid September 9, 2025 12:36
@MikeMcQuaid MikeMcQuaid force-pushed the copilot/fix-keg-relocation-issue branch 2 times, most recently from 5eb2402 to d49b7d9 Compare September 16, 2025 18:12
- Add homebrew_created_file? method to identify service files
- Modify replace_text_in_files to handle Homebrew files separately
- Homebrew-created files now get full path relocation instead of selective relocation

Co-authored-by: MikeMcQuaid <[email protected]>
@MikeMcQuaid MikeMcQuaid force-pushed the copilot/fix-keg-relocation-issue branch from d49b7d9 to 088fa1a Compare September 17, 2025 16:26
@MikeMcQuaid MikeMcQuaid marked this pull request as ready for review September 17, 2025 16:26
Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

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

Makes sense!

@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue Sep 17, 2025
Merged via the queue into main with commit ffdb66a Sep 17, 2025
36 checks passed
@MikeMcQuaid MikeMcQuaid deleted the copilot/fix-keg-relocation-issue branch September 17, 2025 17:21
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.

Keg relocation should always work for homebrew created files
3 participants