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

Load internal json v3 #16638

Merged
merged 8 commits into from
Feb 29, 2024
5 changes: 5 additions & 0 deletions Library/Homebrew/api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,11 @@ def self.tap_from_source_download(path)

Tap.fetch(org, repo)
end

sig { returns(T::Boolean) }
def self.internal_json_v3?
ENV["HOMEBREW_INTERNAL_JSON_V3"].present?
end
end

# @api private
Expand Down
51 changes: 39 additions & 12 deletions Library/Homebrew/api/formula.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,19 +39,24 @@ def self.source_download(formula)

sig { returns(T::Boolean) }
def self.download_and_cache_data!
json_formulae, updated = Homebrew::API.fetch_json_api_file "formula.jws.json"

cache["aliases"] = {}
cache["renames"] = {}
cache["formulae"] = json_formulae.to_h do |json_formula|
json_formula["aliases"].each do |alias_name|
cache["aliases"][alias_name] = json_formula["name"]
end
(json_formula["oldnames"] || [json_formula["oldname"]].compact).each do |oldname|
cache["renames"][oldname] = json_formula["name"]
if Homebrew::API.internal_json_v3?
json_formulae, updated = Homebrew::API.fetch_json_api_file "internal/v3/homebrew-core.jws.json"
overwrite_cache! T.cast(json_formulae, T::Hash[String, T.untyped])
else
json_formulae, updated = Homebrew::API.fetch_json_api_file "formula.jws.json"

cache["aliases"] = {}
cache["renames"] = {}
cache["formulae"] = json_formulae.to_h do |json_formula|
json_formula["aliases"].each do |alias_name|
cache["aliases"][alias_name] = json_formula["name"]
end
(json_formula["oldnames"] || [json_formula["oldname"]].compact).each do |oldname|
cache["renames"][oldname] = json_formula["name"]
end

[json_formula["name"], json_formula.except("name")]
end

[json_formula["name"], json_formula.except("name")]
end

updated
Expand Down Expand Up @@ -88,6 +93,28 @@ def self.all_renames
cache["renames"]
end

sig { returns(Hash) }
def self.tap_migrations
# Not sure that we need to reload here.
unless cache.key?("tap_migrations")
json_updated = download_and_cache_data!
write_names_and_aliases(regenerate: json_updated)
end

cache["tap_migrations"]
end

sig { returns(String) }
def self.tap_git_head
# Note sure we need to reload here.
unless cache.key?("tap_git_head")
json_updated = download_and_cache_data!
write_names_and_aliases(regenerate: json_updated)
end

cache["tap_git_head"]
end

sig { params(regenerate: T::Boolean).void }
def self.write_names_and_aliases(regenerate: false)
download_and_cache_data! unless cache.key?("formulae")
Expand Down
13 changes: 1 addition & 12 deletions Library/Homebrew/dev-cmd/generate-formula-api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,6 @@
Formulary.enable_factory_cache!
Formula.generating_hash!

homebrew_core_tap_hash = {
"tap_git_head" => tap.git_head,
"aliases" => tap.alias_table,
"renames" => tap.formula_renames,
"tap_migrations" => tap.tap_migrations,
"formulae" => {},
}

tap.formula_names.each do |name|
formula = Formulary.factory(name)
name = formula.name
Expand All @@ -77,15 +69,12 @@
File.write("api/formula/#{name}.json", FORMULA_JSON_TEMPLATE)
File.write("formula/#{name}.html", html_template_name)
end

homebrew_core_tap_hash["formulae"][formula.name] =
formula.to_hash_with_variations(hash_method: :to_api_hash)
rescue
onoe "Error while generating data for formula '#{name}'."
raise
end

homebrew_core_tap_json = JSON.generate(homebrew_core_tap_hash)
homebrew_core_tap_json = JSON.generate(tap.to_api_hash)

Check warning on line 77 in Library/Homebrew/dev-cmd/generate-formula-api.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/generate-formula-api.rb#L77

Added line #L77 was not covered by tests
File.write("api/internal/v3/homebrew-core.json", homebrew_core_tap_json) unless args.dry_run?
canonical_json = JSON.pretty_generate(tap.formula_renames.merge(tap.alias_table))
File.write("_data/formula_canonical.json", "#{canonical_json}\n") unless args.dry_run?
Expand Down
7 changes: 7 additions & 0 deletions Library/Homebrew/extend/cachable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,11 @@ def cache
def clear_cache
cache.clear
end

private

sig { params(hash: T::Hash[T.untyped, T.untyped]).void }
def overwrite_cache!(hash)
@cache = hash
end
end
37 changes: 25 additions & 12 deletions Library/Homebrew/formulary.rb
Original file line number Diff line number Diff line change
Expand Up @@ -251,8 +251,8 @@
desc json_formula["desc"]
homepage json_formula["homepage"]
license SPDX.string_to_license_expression(json_formula["license"])
revision json_formula["revision"]
version_scheme json_formula["version_scheme"]
revision json_formula.fetch("revision", 0)
version_scheme json_formula.fetch("version_scheme", 0)

if (urls_stable = json_formula["urls"]["stable"].presence)
stable do
Expand All @@ -262,7 +262,7 @@
using: urls_stable["using"]&.to_sym,
}.compact
url urls_stable["url"], **url_spec
version json_formula["versions"]["stable"]
version Homebrew::API.internal_json_v3? ? json_formula["version"] : json_formula["versions"]["stable"]
sha256 urls_stable["checksum"] if urls_stable["checksum"].present?

instance_exec(:stable, &add_deps)
Expand All @@ -289,7 +289,13 @@
end
end

if (bottles_stable = json_formula["bottle"]["stable"].presence)
bottles_stable = if Homebrew::API.internal_json_v3?
json_formula["bottle"]
else
json_formula["bottle"]["stable"]
end.presence

if bottles_stable
bottle do
if Homebrew::EnvConfig.bottle_domain == HOMEBREW_BOTTLE_DEFAULT_DOMAIN
root_url HOMEBREW_BOTTLE_DEFAULT_DOMAIN
Expand Down Expand Up @@ -373,19 +379,26 @@
&.gsub(HOMEBREW_HOME_PLACEHOLDER, Dir.home)
end

@tap_git_head_string = json_formula["tap_git_head"]
@tap_git_head_string = if Homebrew::API.internal_json_v3?
Homebrew::API::Formula.tap_git_head
else
json_formula["tap_git_head"]
end

def tap_git_head
self.class.instance_variable_get(:@tap_git_head_string)
end

@oldnames_array = json_formula["oldnames"] || [json_formula["oldname"]].compact
def oldnames
self.class.instance_variable_get(:@oldnames_array)
end
unless Homebrew::API.internal_json_v3?
@oldnames_array = json_formula["oldnames"] || [json_formula["oldname"]].compact
def oldnames
self.class.instance_variable_get(:@oldnames_array)

Check warning on line 395 in Library/Homebrew/formulary.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/formulary.rb#L395

Added line #L395 was not covered by tests
end

@aliases_array = json_formula.fetch("aliases", [])
def aliases
self.class.instance_variable_get(:@aliases_array)
@aliases_array = json_formula.fetch("aliases", [])
def aliases
self.class.instance_variable_get(:@aliases_array)

Check warning on line 400 in Library/Homebrew/formulary.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/formulary.rb#L400

Added line #L400 was not covered by tests
end
end

@versioned_formulae_array = json_formula.fetch("versioned_formulae", [])
Expand Down
20 changes: 20 additions & 0 deletions Library/Homebrew/tap.rb
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ def clear_cache
@formula_dir = nil
@cask_dir = nil
@command_dir = nil
@formula_names = nil
@formula_files = nil
@cask_files = nil
@alias_dir = nil
Expand Down Expand Up @@ -1101,6 +1102,8 @@ def tap_migrations
@tap_migrations ||= if Homebrew::EnvConfig.no_install_from_api?
ensure_installed!
super
elsif Homebrew::API.internal_json_v3?
Homebrew::API::Formula.tap_migrations
else
migrations, = Homebrew::API.fetch_json_api_file "formula_tap_migrations.jws.json",
stale_seconds: TAP_MIGRATIONS_STALE_SECONDS
Expand Down Expand Up @@ -1188,6 +1191,23 @@ def formula_files_by_name
hash[name] = Pathname(new_path) if existing_path.nil? || existing_path.to_s.length < new_path.length
end
end

sig { returns(T::Hash[String, T.untyped]) }
def to_api_hash
formulae_api_hash = formula_names.to_h do |name|
formula = Formulary.factory(name)
formula_hash = formula.to_hash_with_variations(hash_method: :to_api_hash)
[name, formula_hash]
end

{
"tap_git_head" => git_head,
"aliases" => alias_table,
"renames" => formula_renames,
"tap_migrations" => tap_migrations,
"formulae" => formulae_api_hash,
}
end
end

# A specialized {Tap} class for homebrew-cask.
Expand Down
144 changes: 144 additions & 0 deletions Library/Homebrew/test/api/internal_tap_json/formula_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
# frozen_string_literal: true

RSpec.describe "Internal Tap JSON -- Formula" do
let(:internal_tap_json) { File.read(TEST_FIXTURE_DIR/"internal_tap_json/homebrew-core.json").chomp }
apainintheneck marked this conversation as resolved.
Show resolved Hide resolved
let(:tap_git_head) { "9977471165641744a829d3e494fa563407503297" }

context "when generating JSON", :needs_macos do
before do
cp_r(TEST_FIXTURE_DIR/"internal_tap_json/homebrew-core", Tap::TAP_DIRECTORY/"homebrew")
apainintheneck marked this conversation as resolved.
Show resolved Hide resolved

# NOTE: Symlinks can't be copied recursively so we create them manually here.
(Tap::TAP_DIRECTORY/"homebrew/homebrew-core").tap do |core_tap|
mkdir(core_tap/"Aliases")
ln_s(core_tap/"Formula/f/fennel.rb", core_tap/"Aliases/fennel-lang")
ln_s(core_tap/"Formula/p/ponyc.rb", core_tap/"Aliases/ponyc-lang")
end
end

it "creates the expected hash" do
api_hash = CoreTap.instance.to_api_hash
apainintheneck marked this conversation as resolved.
Show resolved Hide resolved
api_hash["tap_git_head"] = tap_git_head # tricky to mock

expect(JSON.pretty_generate(api_hash)).to eq(internal_tap_json)
end
end

context "when loading JSON" do
before do
ENV["HOMEBREW_INTERNAL_JSON_V3"] = "1"
ENV.delete("HOMEBREW_NO_INSTALL_FROM_API")

allow(Homebrew::API).to receive(:fetch_json_api_file)
.with("internal/v3/homebrew-core.jws.json")
.and_return([JSON.parse(internal_tap_json), false])

# `Tap.reverse_tap_migrations_renames` looks for renames in every
# tap so `CoreCaskTap.tap_migrations` gets called and tries to
# fetch stuff from the API. This just avoids errors.
allow(Homebrew::API).to receive(:fetch_json_api_file)
.with("cask_tap_migrations.jws.json", anything)
.and_return([{}, false])

# To allow `formula_names.txt` to be written to the cache.
(HOMEBREW_CACHE/"api").mkdir

Homebrew::API::Formula.clear_cache
end

after do
Homebrew::API::Formula.clear_cache
end

it "loads tap aliases" do
expect(CoreTap.instance.alias_table).to eq({
"fennel-lang" => "fennel",
"ponyc-lang" => "ponyc",
})
end

it "loads formula renames" do
expect(CoreTap.instance.formula_renames).to eq({
"advancemenu" => "advancemame",
"amtk" => "libgedit-amtk",
"annie" => "lux",
"antlr2" => "antlr@2",
"romanesco" => "fennel",
})
end

it "loads tap migrations" do
expect(CoreTap.instance.tap_migrations).to eq({
"adobe-air-sdk" => "homebrew/cask",
"android-ndk" => "homebrew/cask",
"android-platform-tools" => "homebrew/cask",
"android-sdk" => "homebrew/cask",
"app-engine-go-32" => "homebrew/cask/google-cloud-sdk",
})
end

it "loads tap git head" do
expect(Homebrew::API::Formula.tap_git_head)
.to eq(tap_git_head)
end

context "when loading formulae" do
let(:fennel_metadata) do
{
"dependencies" => ["lua"],
"desc" => "Lua Lisp Language",
"full_name" => "fennel",
"homepage" => "https://fennel-lang.org",
"license" => "MIT",
"name" => "fennel",
"ruby_source_path" => "Formula/f/fennel.rb",
"tap" => "homebrew/core",
"tap_git_head" => tap_git_head,
"versions" => { "bottle"=>true, "head"=>nil, "stable"=>"1.4.0" },
}
end

let(:ponyc_metadata) do
{
"desc" => "Object-oriented, actor-model, capabilities-secure programming language",
"full_name" => "ponyc",
"homepage" => "https://www.ponylang.io/",
"license" => "BSD-2-Clause",
"name" => "ponyc",
"ruby_source_path" => "Formula/p/ponyc.rb",
"tap" => "homebrew/core",
"tap_git_head" => tap_git_head,
# TODO: improve this API before we ship internal API v3 to users
"uses_from_macos" => [{ "llvm"=>[:build, :test] }, "zlib"],
"uses_from_macos_bounds" => [{}, {}],
Comment on lines +112 to +113
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to increase up scope here potentially (and can be in another PR) but: this API is fairly awful due to backwards compatibility concerns we don't have for v3 so would be nice to address. CC @Bo98 for thoughts on what that should look like.

Copy link
Member

@Bo98 Bo98 Feb 19, 2024

Choose a reason for hiding this comment

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

Yeah we were going to cycle back and clean this up. I can open a PR with suggestions (probably easier after merging this PR though)

Copy link
Member

Choose a reason for hiding this comment

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

Cool: given that: fine to merge with this as-is in this PR 👍🏻. A TODO or something might be nice though?

Copy link
Member

Choose a reason for hiding this comment

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

Can a TODO be added here please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left a note in the original issue thread: #16410 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems weird to leave a comment when any potential changes are exploratory. We haven't decided on the form that we want yet.

Copy link
Member

Choose a reason for hiding this comment

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

@apainintheneck It's a hard TODO because we're not going to ship the internal JSON v3 without changing this. I'm fine on not blocking the PR on this stuff but I would really like something beyond a comment in another issue to track this in the actual code.

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
"uses_from_macos" => [{ "llvm"=>[:build, :test] }, "zlib"],
"uses_from_macos_bounds" => [{}, {}],
# TODO: improve this API before we ship internal API v3 to users
"uses_from_macos" => [{ "llvm"=>[:build, :test] }, "zlib"],
"uses_from_macos_bounds" => [{}, {}],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@apainintheneck It's a hard TODO because we're not going to ship the internal JSON v3 without changing this. I'm fine on not blocking the PR on this stuff but I would really like something beyond a comment in another issue to track this in the actual code.

I'm fine with the comment but this is news to me. I didn't know that this was that important either way.

Copy link
Member

Choose a reason for hiding this comment

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

Apologies for my unclear communication! I'm trying to get the balance right between being overly pedantic/demotivating and blocking individual PRs vs. making clear what's required before we ship to users. Great work getting this shipped, thanks @apainintheneck ❤️

"versions" => { "bottle"=>true, "head"=>nil, "stable"=>"0.58.1" },
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
"versions" => { "bottle"=>true, "head"=>nil, "stable"=>"0.58.1" },
"versions" => { "bottle"=>true, "stable"=>"0.58.1" },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I can probably compact or avoid adding that field in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind, this only exists in the public facing JSON and should already be omitted from the internal one. I'm just using the public JSON here for testing because it exercises more parts of the formula code.

Copy link
Member

Choose a reason for hiding this comment

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

Yeh, I think it'd be preferable to use the internal JSON and to get full coverage of the formula code through the input JSON.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh, I think it'd be preferable to use the internal JSON and to get full coverage of the formula code through the input JSON.

Sorry, I don't know what you mean by this. How can we get full coverage of the formula code through the input JSON? What input JSON are we referring to here?

Copy link
Member

Choose a reason for hiding this comment

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

I'm saying it seems very weird to be to use the non-internal public JSON for testing the internal JSON format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not that weird. It essentially mimics brew readall.

Copy link
Member

Choose a reason for hiding this comment

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

@apainintheneck Ok but aren't these JSON formats going to diverge significantly?

I think I might just need an ELI5 treatment here, sorry.

Copy link
Member

@Bo98 Bo98 Feb 27, 2024

Choose a reason for hiding this comment

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

This is basically a fancy to_hash test. We could do it as a series of checks instead, though would be multiple expects in a single check so might be long:

expect(formula.name).to eq("something")
expect(formula.version.to eq("1.0")
expect(formula.revision).to be_zero
# ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#to_hash is a bit better than doing a bunch of individual checks since it will end up calling more methods internally to fill out the fields we're not checking.

The goal here is not to validate the #to_hash method but instead to be reasonably confident that the formula loads without failing. #to_hash is just an easy way to take a peek inside the formula instance and run some arbitrary methods along the way. I'm open to alternatives that would be clearer, briefer or provide more assurance of correctness. The current test is not comprehensive at all.

#to_api_hash is already being exercised by a different test where we generate the JSON API for the core formula tap.

}
end

it "loads fennel" do
fennel = Formulary.factory("fennel")
expect(fennel.to_hash).to include(**fennel_metadata)
end

it "loads fennel from rename" do
fennel = Formulary.factory("romanesco")
expect(fennel.to_hash).to include(**fennel_metadata)
end

it "loads fennel from alias" do
fennel = Formulary.factory("fennel-lang")
expect(fennel.to_hash).to include(**fennel_metadata)
end

it "loads ponyc" do
ponyc = Formulary.factory("ponyc")
expect(ponyc.to_hash).to include(**ponyc_metadata)
end

it "loads ponyc from alias" do
ponyc = Formulary.factory("ponyc-lang")
expect(ponyc.to_hash).to include(**ponyc_metadata)
end
end
end
end