Use cached json API file for formulae and cask specified paths

[Rylan12]

• 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?

---

This PR aligns Formula#specified_path and Cask#sourcefile_path with each other. Now, when loading from the API, these methods return the file path of the cached JSON API file that was used by Homebrew::API when loading. If the formula/cask was loaded from a ruby file, that file is returned. If the cask was loaded directly from a different JSON file, the path to that file is returned, instead.

Addressing a discussion point from #17554 (comment): both ruby_source_path and ruby_source_checksum are manually set when loading from the API, so those methods don't need to be adjusted with this change.

Loading a formula

From the API

Formulary.path("hello")
# => #<Pathname:/opt/homebrew/Library/Taps/homebrew/homebrew-core/Formula/h/hello.rb>

Formulary.factory("hello").path
# => #<Pathname:/opt/homebrew/Library/Taps/homebrew/homebrew-core/Formula/h/hello.rb>

Formulary.factory("hello").specified_path
# => #<Pathname:/Users/rylanpolster/Library/Caches/Homebrew/api/formula.jws.json>

Formulary.factory("hello").loaded_from_api?
# => true

Without the API

Formulary.path("hello")
# => #<Pathname:/opt/homebrew/Library/Taps/homebrew/homebrew-core/Formula/h/hello.rb>

Formulary.factory("hello").path
# => #<Pathname:/opt/homebrew/Library/Taps/homebrew/homebrew-core/Formula/h/hello.rb>

Formulary.factory("hello").specified_path
# => #<Pathname:/opt/homebrew/Library/Taps/homebrew/homebrew-core/Formula/h/hello.rb>

Formulary.factory("hello").loaded_from_api?
# => false

Directly from a ruby file

Formulary.path("/tmp/hello.rb")
# => #<Pathname:/tmp/hello.rb>

Formulary.factory("/tmp/hello.rb").path
# => #<Pathname:/tmp/hello.rb

Formulary.factory("/tmp/hello.rb").specified_path
# => #<Pathname:/tmp/hello.rb

Formulary.factory("/tmp/hello.rb").loaded_from_api?
# => false

Loading a cask

From the API

Cask::CaskLoader.path("slack")
# => #<Pathname:/opt/homebrew/Library/Taps/homebrew/homebrew-cask/Casks/s/slack.rb>

Cask::CaskLoader.load("slack").sourcefile_path
# => #<Pathname:/Users/rylanpolster/Library/Caches/Homebrew/api/cask.jws.json>

Cask::CaskLoader.load("slack").loaded_from_api?
# => true

Without the API

Cask::CaskLoader.path("slack")
# => #<Pathname:/opt/homebrew/Library/Taps/homebrew/homebrew-cask/Casks/s/slack.rb>

Cask::CaskLoader.load("slack").sourcefile_path
# => #<Pathname:/opt/homebrew/Library/Taps/homebrew/homebrew-cask/Casks/s/slack.rb>

Cask::CaskLoader.load("slack").loaded_from_api?
# => false

Directly from a ruby file

Cask::CaskLoader.path("/tmp/slack.rb")
# => #<Pathname:/tmp/slack.rb>

Cask::CaskLoader.load("/tmp/slack.rb").sourcefile_path
# => #<Pathname:/tmp/slack.rb>

Cask::CaskLoader.load("/tmp/slack.rb").loaded_from_api?
# => false

Directly from a json file

Cask::CaskLoader.path("/tmp/slack.json")
# => #<Pathname:/tmp/slack.json>

Cask::CaskLoader.load("/tmp/slack.json").sourcefile_path
# => #<Pathname:/tmp/slack.json>

Cask::CaskLoader.load("/tmp/slack.json").loaded_from_api?
# => true

[apainintheneck]

I'm seeing some places where Cask::Cask#sourcefile_path relies on the old behavior. Have you checked where that method is used throughout the codebase?

brew/Library/Homebrew/cmd/info.rb

Lines 252 to 254 in 5e7765c

	if cask.sourcefile_path.blank?
	return "#{cask.tap.default_remote}/blob/HEAD/#{cask.tap.relative_cask_path(cask.token)}"
	end

It seems like there is an assumption that casks should be loadable from the source file path as well. This kind of breaks that assumption doesn't it. In practice, it shouldn't matter since a lot of those places are wrapped in without API blocks but it does make the code harder to reason about and possibly more error prone.

brew/Library/Homebrew/dev-cmd/bump.rb

Line 346 in 5e7765c

loaded_formula_or_cask = Cask::CaskLoader.load(formula_or_cask.sourcefile_path)

I wonder if there's a better place to apply this change so that the path is consistent in brew info, the JSON API and the install receipt but doesn't break assumptions that are already present in the codebase.

[apainintheneck]

I guess you could argue that that assumption has already been broken for a while though since it's been nullable this whole time.

[apainintheneck]

The change to Formula#specified_path seems reasonable. The only place it's really used has a check for formulas that have been loaded from the API.

brew/Library/Homebrew/formula_installer.rb

Lines 1109 to 1136 in 5e7765c

	def post_install_formula_path
	# Use the formula from the keg when any of the following is true:
	# * We're installing from the JSON API
	# * We're installing a local bottle file
	# * The formula doesn't exist in the tap (or the tap isn't installed)
	# * The formula in the tap has a different `pkg_version``.
	#
	# In all other cases, including if the formula from the keg is unreadable
	# (third-party taps may `require` some of their own libraries) or if there
	# is no formula present in the keg (as is the case with very old bottles),
	# use the formula from the tap.
	keg_formula_path = formula.opt_prefix/".brew/#{formula.name}.rb"
	return keg_formula_path if formula.loaded_from_api?
	return keg_formula_path if formula.local_bottle_path.present?
	tap_formula_path = formula.specified_path
	return keg_formula_path unless tap_formula_path.exist?
	begin
	keg_formula = Formulary.factory(keg_formula_path)
	tap_formula = Formulary.factory(tap_formula_path)
	return keg_formula_path if keg_formula.pkg_version != tap_formula.pkg_version
	tap_formula_path
	rescue FormulaUnavailableError, FormulaUnreadableError
	tap_formula_path
	end
	end

[apainintheneck]

Maybe it'd make more sense to have two methods that distinguish diagnostic paths, like those used in brew info and the install receipt, from loadable paths, like those fed to the cask loader.

[MikeMcQuaid]

This makes sense to me. I agree with @apainintheneck in might be nice to differentiate between "location on disk this was loaded from" and "where we'd expect this to be" but: that can (and should) be in a follow-up PR.

One optional naming tweak and this is good to go (without a rereview being needed).

[MikeMcQuaid]

Maybe it'd make more sense to have two methods that distinguish diagnostic paths, like those used in brew info and the install receipt, from loadable paths, like those fed to the cask loader.

Personally: I'm not sure we should ever be outputting non-loadable paths to users.

I agree in the receipts, if we want/need two paths, to always have a differentiation between "loaded from" and "expected location on disk" is valuable but, even then, I wonder about the merits of including paths that don't/won't exist here. Feels unnecessarily confusing.

[Rylan12]

You're right about the brew info case, I somehow missed that when testing. I've pushed a fix to address that (and the rename). As far as I can tell, the remaining cases are all gated by API calls, but I hear what you're saying about it not being clear.

I agree, it does kind of seem like there should be two distinct path methods (which is also what we have in formulae with #path and #specified_path). Maybe in a follow-up PR I can add #specified_path as an additional cask method? I don't really know how often it would be used, though, so it may not be worth it. Open to thoughts

[apainintheneck]

I'm happy with this approach. Worst-case scenario we get an error message because someone tried to load a cask using the source file path outside of a without API block. That error message should be explanatory enough and easy to fix. Adding another method wouldn't really help things. Thanks for humoring my idea while I thought it out though.