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
create: Fix getting name from GitHub archives #16238
Conversation
I have no idea why the test fails. Its execution path with Failures:
1) brew create creates a new Formula file for a given URL
Failure/Error: expect(formula_file).to exist
expected #<Pathname:/tmp/homebrew-tests-20231120-3549-dtiz3r/prefix/Library/Taps/homebrew/homebrew-core/Formula/testball.rb> to exist
# ./test/dev-cmd/create_spec.rb:15:in `block (2 levels) in <top (required)>'
# ./test/support/helper/spec/shared_context/integration_test.rb:49:in `block (2 levels) in <top (required)>'
Randomized with seed 62719 |
The failure is caused by the move of |
Okay, It looks like brew/Library/Homebrew/formula_creator.rb Line 19 in ad1bb17
But given that
|
All right, it fails, because |
There was a problem hiding this 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, nice work so far!
Okay, It looks like fc.url setter is way too smart
The opposite, actually: I'd rather than this logic moved into FormulaCreator
for formulae.
Is it a problem for casks for you personally or just formulae? If it's just a formulae problem: just handle this all in FormulaCreator. If also casks: it'd be good to move the logic to something closer to FormulaCreator but for casks instead.
I don't have Mac, so I don't have experience with casts. It just looks like the logic to get the name from an URL is the same for creating the cask and the formula. |
Ok, just scope it to formulae only then and put logic in |
I moved the I also left |
I a rest. 10 hours is too much for fixing such a small issue. :D |
85f4f7e
to
467830a
Compare
Library/Homebrew/formula_creator.rb
Outdated
stem.rpartition("=").last | ||
elsif url =~ %r{github\.com/\S+/(\S+)/(archive|releases)/} | ||
Regexp.last_match(1) | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the else
not be the original behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand it right. else
is the original behavior from here.
brew/Library/Homebrew/formula_creator.rb
Lines 33 to 34 in 1b8207b
else | |
@name = path.basename.to_s[/(.*?)[-_.]?#{Regexp.escape(path.version.to_s)}/, 1] |
467830a
to
999f1a2
Compare
999f1a2
to
7bcca28
Compare
@MikeMcQuaid rebase was unfeasible, so I just overwrote the branch with copy-pasted content after the review. It is now ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I may have missed a discussion on this (although I don't see one in the comments). Could the issue be avoided by reordering the name assignment and the fc.parse_url
call?:
fc = FormulaCreator.new(...)
fc.parse_url
if args.set_name.blank? # So that this will run even if the name is detected from the URL but wasn't passed to `brew create`
print "Formula name [#{fc.name}]: " # May need to handle `fc.name` being blank better...
fc.name = __gets || fc.name
end
(I've not tested the above)
It feels like assigning @name
to a value that comes from the URL should happen within parse_url
rather than its own method to be called in basically the same place.
I believe the root cause of the bug is that the code in It makes sense to move |
@Rylan12 thanks for the suggestion. The code looks better now, with one less bug that prevented GitLab user name from being parsed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Couple more things.
Library/Homebrew/formula_creator.rb
Outdated
name = if stem.start_with? "index.cgi" | ||
# parsing gitweb URLs http://www.codesrc.com/gitweb/index.cgi?p=libzipper.git;a=summary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's specifically worth checking for the domain name here or more bits e.g. gitweb
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a default Git interface https://wiki.archlinux.org/title/gitweb so checking domain name would exclude other installations.
Checking for gitweb
specifically.. well, the stem
variable is still needed for other rules, so the would be a bit redundant. I don't think there are many repo browser installations with exposed index.cgi
which are not gitweb
. In any case, if somebody reports new URL, it is easy to patch given we've got something to test against.
@MikeMcQuaid looks like some problem with linter. The commit Style/RedundantSelf which fixes https://github.com/Homebrew/brew/actions/runs/7091523800/job/19300839843?pr=16238#step:6:15 breaks the code. |
@MikeMcQuaid I had to revert the suggestion to call static method as |
it "parses names from URL correctly" do | ||
t = described_class.name_from_url("https://github.com/abitrolly/lapce/archive/v0.3.0.tar.gz") | ||
expect(t).to eq("lapce") | ||
t = described_class.name_from_url("http://www.codesrc.com/gitweb/index.cgi?p=libzipper.git;a=summary") | ||
expect(t).to eq("libzipper") | ||
t = described_class.name_from_url("https://github.com/abitrolly/lapce.git") | ||
expect(t).to eq("lapce") | ||
t = described_class.name_from_url("https://github.com/stella-emu/stella/releases/download/6.7/stella-6.7-src.tar.xz") | ||
expect(t).to eq("stella") | ||
t = described_class.name_from_url("http://digit-labs.org/files/tools/synscan/releases/synscan-5.02.tar.gz") | ||
expect(t).to eq("synscan") | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please split this into one URL/expectation per test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean split it into five it "parses URL ..."
blocks ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
1c8c3db
to
1f39e83
Compare
Done. Rebased and squashed a bit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work here @abitrolly. Thanks for all your patience, hard work and back and forth here. Hope this is your first PR of many ❤️
`brew create https://github.com/lapce/lapce/archive/v0.3.0.tar.gz` was getting the wrong name 'v3.0.0' from the URL Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>
1f39e83
to
6afd15a
Compare
brew create https://github.com/lapce/lapce/archive/v0.3.0.tar.gz
currently gets the wrong namev3.0.0
from the URL. The PR fixes that.brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?