create: Fix getting name from GitHub archives #16238

abitrolly · 2023-11-20T09:11:53Z

brew create https://github.com/lapce/lapce/archive/v0.3.0.tar.gz currently gets the wrong name v3.0.0 from the URL. The PR fixes that.

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?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

abitrolly · 2023-11-20T11:14:28Z

I have no idea why the test fails. Its execution path with --set-name should avoid modifications that I've made. Need help with that.

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

abitrolly · 2023-11-20T12:16:15Z

The failure is caused by the move of fc.url to the top of the create function. I see no reasons why it should fail. Really weird.

abitrolly · 2023-11-20T12:45:43Z

Okay, It looks like fc.url setter is way too smart

brew/Library/Homebrew/formula_creator.rb

Line 19 in ad1bb17

def url=(url)

But given that FormulaCreator is only used once, it should be possible to remove URL parsing from it. Because that logic is also needed when creating casks.

$ rg FormulaCreator
Library/Homebrew/sorbet/rbi/hidden-definitions/hidden.rbi
4508:class Homebrew::FormulaCreator

Library/Homebrew/formula_creator.rb
11:  class FormulaCreator

Library/Homebrew/dev-cmd/create.rb
143:    fc = FormulaCreator.new(args)

abitrolly · 2023-11-20T13:05:29Z

All right, it fails, because url= setter does too much and also sets version, and if it placed to the top, then the version is overwritten by fc.version = args.set_version line.

MikeMcQuaid

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.

abitrolly · 2023-11-20T15:30:45Z

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.

MikeMcQuaid · 2023-11-20T15:33:44Z

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 FormulaCreator, thanks!

Library/Homebrew/dev-cmd/create.rb

abitrolly · 2023-11-20T18:47:32Z

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 FormulaCreator, thanks!

I moved the url2name function to formula_creator.rb to keep it stateless. If it was a stateless function from the start, it wouldn't take me 9 hours to add a new rule to it. It is 9 hours given that I am already familiar with the codebase and that isn't my first attempt to fix the issue.

I also left create --cask alone, scoping the change to formulas only.

abitrolly · 2023-11-20T18:56:17Z

I a rest. 10 hours is too much for fixing such a small issue. :D

Library/Homebrew/dev-cmd/create.rb

Library/Homebrew/formula_creator.rb

MikeMcQuaid · 2023-11-21T15:47:27Z

Library/Homebrew/formula_creator.rb

+      stem.rpartition("=").last
+    elsif url =~ %r{github\.com/\S+/(\S+)/(archive|releases)/}
+      Regexp.last_match(1)
+    else


Should the else not be the original behaviour?

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]

abitrolly · 2023-12-01T19:38:20Z

@MikeMcQuaid rebase was unfeasible, so I just overwrote the branch with copy-pasted content after the review. It is now ready.

Rylan12

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.

abitrolly · 2023-12-04T07:17:21Z

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 parse_url was too magic to understand, so the rule that broke the parsing was added in the wrong place. It was also impossible to cover by tests.

It makes sense to move name_from_url call to parse_url though.

abitrolly · 2023-12-04T07:58:11Z

@Rylan12 thanks for the suggestion. The code looks better now, with one less bug that prevented GitLab user name from being parsed.

MikeMcQuaid

Looking good! Couple more things.

Library/Homebrew/formula_creator.rb

MikeMcQuaid · 2023-12-04T18:38:54Z

Library/Homebrew/formula_creator.rb

+      name = if stem.start_with? "index.cgi"
+        # parsing gitweb URLs http://www.codesrc.com/gitweb/index.cgi?p=libzipper.git;a=summary


I wonder if it's specifically worth checking for the domain name here or more bits e.g. gitweb?

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.

Library/Homebrew/formula_creator.rb

abitrolly · 2023-12-04T19:48:06Z

@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.

abitrolly · 2023-12-05T05:10:09Z

@MikeMcQuaid I had to revert the suggestion to call static method as self.name_from_url - it didn't work and subsequent linter errors were misleading - rubocop/rubocop#12512 (comment)

Library/Homebrew/formula_creator.rb

MikeMcQuaid · 2023-12-05T13:20:34Z

Library/Homebrew/test/formula_creator_spec.rb

+  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


Please split this into one URL/expectation per test

Do you mean split it into five it "parses URL ..." blocks ?

abitrolly · 2023-12-05T15:40:33Z

Done. Rebased and squashed a bit.

MikeMcQuaid

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>

MikeMcQuaid reviewed Nov 20, 2023

View reviewed changes

Library/Homebrew/dev-cmd/create.rb Outdated Show resolved Hide resolved

abitrolly force-pushed the create-urlparse branch from 85f4f7e to 467830a Compare November 21, 2023 10:08

abitrolly mentioned this pull request Nov 21, 2023

formula_creator: Remove path attr to reduce code complexity #16244

Merged

7 tasks

MikeMcQuaid reviewed Nov 21, 2023

View reviewed changes

abitrolly force-pushed the create-urlparse branch from 467830a to 999f1a2 Compare November 23, 2023 16:22

abitrolly mentioned this pull request Nov 27, 2023

formula_creator: move initial CLI values into constructor #16260

Merged

7 tasks

abitrolly marked this pull request as draft November 30, 2023 20:50

abitrolly force-pushed the create-urlparse branch from 999f1a2 to 7bcca28 Compare December 1, 2023 19:23

abitrolly marked this pull request as ready for review December 1, 2023 19:34

Rylan12 reviewed Dec 4, 2023

View reviewed changes

MikeMcQuaid reviewed Dec 4, 2023

View reviewed changes

MikeMcQuaid reviewed Dec 5, 2023

View reviewed changes

Library/Homebrew/formula_creator.rb Outdated Show resolved Hide resolved

Library/Homebrew/formula_creator.rb Outdated Show resolved Hide resolved

Library/Homebrew/formula_creator.rb Show resolved Hide resolved

MikeMcQuaid reviewed Dec 5, 2023

View reviewed changes

abitrolly force-pushed the create-urlparse branch 2 times, most recently from 1c8c3db to 1f39e83 Compare December 5, 2023 15:34

MikeMcQuaid approved these changes Dec 5, 2023

View reviewed changes

MikeMcQuaid enabled auto-merge December 5, 2023 18:53

create: Fix getting name from GitHub archives

6afd15a

`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>

MikeMcQuaid force-pushed the create-urlparse branch from 1f39e83 to 6afd15a Compare December 5, 2023 19:50

MikeMcQuaid merged commit b9dce73 into Homebrew:master Dec 5, 2023
26 checks passed

abitrolly deleted the create-urlparse branch December 6, 2023 00:49

github-actions bot added the outdated PR was locked due to age label Jan 6, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jan 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create: Fix getting name from GitHub archives #16238

create: Fix getting name from GitHub archives #16238

abitrolly commented Nov 20, 2023 •

edited

abitrolly commented Nov 20, 2023

abitrolly commented Nov 20, 2023

abitrolly commented Nov 20, 2023

abitrolly commented Nov 20, 2023

MikeMcQuaid left a comment

abitrolly commented Nov 20, 2023

MikeMcQuaid commented Nov 20, 2023

abitrolly commented Nov 20, 2023

abitrolly commented Nov 20, 2023

MikeMcQuaid Nov 21, 2023

abitrolly Nov 23, 2023

abitrolly commented Dec 1, 2023

Rylan12 left a comment

abitrolly commented Dec 4, 2023

abitrolly commented Dec 4, 2023

MikeMcQuaid left a comment

MikeMcQuaid Dec 4, 2023

abitrolly Dec 4, 2023

abitrolly commented Dec 4, 2023

abitrolly commented Dec 5, 2023

MikeMcQuaid Dec 5, 2023

abitrolly Dec 5, 2023

MikeMcQuaid Dec 5, 2023

abitrolly commented Dec 5, 2023

MikeMcQuaid left a comment

	else
	@name = path.basename.to_s[/(.*?)[-_.]?#{Regexp.escape(path.version.to_s)}/, 1]

		name = if stem.start_with? "index.cgi"
		# parsing gitweb URLs http://www.codesrc.com/gitweb/index.cgi?p=libzipper.git;a=summary

create: Fix getting name from GitHub archives #16238

create: Fix getting name from GitHub archives #16238

Conversation

abitrolly commented Nov 20, 2023 • edited

abitrolly commented Nov 20, 2023

abitrolly commented Nov 20, 2023

abitrolly commented Nov 20, 2023

abitrolly commented Nov 20, 2023

MikeMcQuaid left a comment

Choose a reason for hiding this comment

abitrolly commented Nov 20, 2023

MikeMcQuaid commented Nov 20, 2023

abitrolly commented Nov 20, 2023

abitrolly commented Nov 20, 2023

MikeMcQuaid Nov 21, 2023

Choose a reason for hiding this comment

abitrolly Nov 23, 2023

Choose a reason for hiding this comment

abitrolly commented Dec 1, 2023

Rylan12 left a comment

Choose a reason for hiding this comment

abitrolly commented Dec 4, 2023

abitrolly commented Dec 4, 2023

MikeMcQuaid left a comment

Choose a reason for hiding this comment

MikeMcQuaid Dec 4, 2023

Choose a reason for hiding this comment

abitrolly Dec 4, 2023

Choose a reason for hiding this comment

abitrolly commented Dec 4, 2023

abitrolly commented Dec 5, 2023

MikeMcQuaid Dec 5, 2023

Choose a reason for hiding this comment

abitrolly Dec 5, 2023

Choose a reason for hiding this comment

MikeMcQuaid Dec 5, 2023

Choose a reason for hiding this comment

abitrolly commented Dec 5, 2023

MikeMcQuaid left a comment

Choose a reason for hiding this comment

abitrolly commented Nov 20, 2023 •

edited