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

Add tap file patch diff #15841

Closed
wants to merge 2 commits into from
Closed

Conversation

gaborbernat
Copy link

@gaborbernat gaborbernat commented Aug 7, 2023

Resolves #15838

Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
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 the PR @gaborbernat, much appreciated, looking good so far.

Could you add another test to Library/Homebrew/test/patching_spec.rb so we can see the DSL in action?

Comment on lines 175 to 183
class TapFilePatch < EmbeddedPatch
def initialize(strip, tap_file)
super(strip)
@str = File.open(tap_file).read
end

def contents
@str
end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class TapFilePatch < EmbeddedPatch
def initialize(strip, tap_file)
super(strip)
@str = File.open(tap_file).read
end
def contents
@str
end
class TapFilePatch < EmbeddedPatch
attr_reader :contents
sig { params(strip: T.any(Symbol, String), tap_file: T.any(String, Pathname)).void }
def initialize(strip, tap_file)
super(strip)
@contents = File.open(tap_file).read
end

Note: I expect that the File.open may need to be a little more involved here to be able to find the patch in a suitable location. The ideal location in a tap would feel like a Patches/ subdirectory in the root. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

I personally would prefer to keep the patches alongside the formula in the filesystem/editor... so would prefer not too have a patches folder. Instead Perhaps we can have:

Formula.rb
Formula.patch_name.diff

?

Copy link
Member

Choose a reason for hiding this comment

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

If that's all you need then you can probably already do something like

patch do
  url "file://#{__dir__}/Formula.patch_name.diff"
end

without having to make any changes to the formula DSL.

Copy link
Author

Choose a reason for hiding this comment

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

Does #{__dir__} gets replaced with the formulas folder name?

Copy link
Member

Choose a reason for hiding this comment

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

yes

Copy link
Author

Choose a reason for hiding this comment

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

Ah then never mind.

@MikeMcQuaid
Copy link
Member

Some failing style checks (https://github.com/Homebrew/brew/actions/runs/5788904612/job/15688846613?pr=15841#step:6:12) but brew style --fix should sort these out easily enough.

Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
@gaborbernat
Copy link
Author

Could you add another test to Library/Homebrew/test/patching_spec.rb so we can see the DSL in action?

Would love to, but given I have no ruby experience can you point me towards where I can steal it from? 😆

@MikeMcQuaid
Copy link
Member

Would love to, but given I have no ruby experience can you point me towards where I can steal it from? 😆

@gaborbernat go in that file, copy-paste an existing test and adjust it to use your new patch method instead of the existing one.

@gaborbernat gaborbernat closed this Aug 8, 2023
@github-actions github-actions bot added the outdated PR was locked due to age label Sep 8, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 8, 2023
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.

Ability to add patches alongside the formula
3 participants