cmd/update-report: improve formula file validation #15489
Conversation
Library/Homebrew/tap.rb
Outdated
| # returns true if given path would present a {Formula} file in this {Tap}. | ||
| # accepts both absolute path and relative path (relative to this {Tap}'s path) | ||
| # @private | ||
| sig { params(file: T.any(String, Pathname)).returns(T::Boolean) } | ||
| def formula_file?(file) | ||
| file = Pathname.new(file) unless file.is_a? Pathname | ||
| file = file.expand_path(path) | ||
| return false unless ruby_file?(file) | ||
| return false if cask_file?(file) | ||
|
|
||
| file.to_s.start_with?("#{formula_dir}/") | ||
| end | ||
|
|
||
| # returns true if given path would present a {Cask} file in this {Tap}. | ||
| # accepts both absolute path and relative path (relative to this {Tap}'s path) | ||
| # @private | ||
| sig { params(file: T.any(String, Pathname)).returns(T::Boolean) } | ||
| def cask_file?(file) | ||
| file = Pathname.new(file) unless file.is_a? Pathname | ||
| file = file.expand_path(path) | ||
| return false unless ruby_file?(file) | ||
|
|
||
| file.to_s.start_with?("#{cask_dir}/") | ||
| end | ||
|
|
There was a problem hiding this comment.
These were only used in the update report and Tap#formula_files above which has been updated.
There was a problem hiding this comment.
Tap#cask_file? is used for cask CI currently. There it is also used in combination with git diff, so may be best to move the git diff logic into Tap, so it can be used to get all changed files between two commits.
There was a problem hiding this comment.
Could you point out where specifically it's used in cask CI? I couldn't find it with a quick grep of the cask repo.
Both of these were marked @private so I didn't check other repos but now that you mentioned it I looked and one of them is used in the test bot repo.
/u/l/H/L/T/h/homebrew-test-bot (master|✔) $ git grep -F '.cask_file?'
/u/l/H/L/T/h/homebrew-test-bot (master|✔) [1]$ git grep -F '.formula_file?'
lib/tests/formulae_detect.rb: next unless tap.formula_file?(file)
There was a problem hiding this comment.
I just rebased this to add back in the .cask_file? and .formula_file? methods because they are used in other repos. I made both of them more restrictive though. The use of .formula_file? in test bot would need to be changed first though but the current method already takes a string so that shouldn't be too hard.
The idea of combining the git diff logic is an interesting idea. I'd have to look to see how much logic is shared there between different use cases.
Currently, ruby files that are not in the Cask directory are considered to be formulae if a Formula or HomebrewFormula directory doesn't exist which doesn't make sense. We know that these should only be in a few directories so we can check for that explicitly. Beyond that the `Tap#cask_file?` and `Tap.formula_file?` methods were only used inside update-report so it doesn't make sense to turn them into pathnames and expand things when we know that each string will be a relative path from a tap that we can just check with a regex. This change will stop other tap changes like new commands or changes to other directories fke lib/ rom showing up as new formulae.
apainintheneck
force-pushed
the
fix-cask-formula-file-validation
branch
from
eaaf0ed to
6f7181b
Compare
| if official? | ||
| %r{^Formula(/[^/]+)+\.rb$} | ||
| else | ||
| %r{^Formula/[^/]+\.rb$} | ||
| end |
There was a problem hiding this comment.
Not sure if this difference is actually meaningly in most cases.
There was a problem hiding this comment.
Does that mean if e.g. Formula/ exists, any Ruby files in the root are not considered formula files?
They are also used in https://github.com/homebrew/homebrew-test-bot/ |
| else | ||
| %r{^Formula/[^/]+\.rb$} | ||
| end | ||
| when "HomebrewFormula" |
There was a problem hiding this comment.
IMO we should consider deprecating both HomebrewFormula and having formula files in the root of a tap.
| @@ -173,7 +173,6 @@ def setup_completion(link:) | |||
| expect(homebrew_foo_tap.tap_migrations).to eq("removed-formula" => "homebrew/foo") | |||
| expect(homebrew_foo_tap.command_files).to eq([cmd_file]) | |||
| expect(homebrew_foo_tap.to_hash).to be_a(Hash) | |||
| expect(homebrew_foo_tap).to have_formula_file(formula_file) | |||
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?Currently, ruby files that are not in the Cask directory are considered to be formulae which doesn't make sense. We know that these should only be in a few directories so we can check for that explicitly.
Beyond that the
Tap#cask_file?andTap.formula_file?methods were only used inside update-report so it doesn't make sense to turn them into pathnames and expand things when we know that each string will be a relative path from a tap that we can just check with a regex.This change will stop other tap changes like new commands or changes to other directories from showing up as new formulae.