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.

Sign up for GitHub

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

Jump to bottom

Stop including flight block source in json #14611

Merged
merged 1 commit into from Feb 14, 2023
Merged

Stop including flight block source in json #14611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Expand Up @@ -111,6 +111,7 @@
**/vendor/bundle/ruby/*/gems/jaro_winkler-*/
**/vendor/bundle/ruby/*/gems/json-*/
**/vendor/bundle/ruby/*/gems/json_schemer-*/
**/vendor/bundle/ruby/*/gems/method_source-*/
**/vendor/bundle/ruby/*/gems/mime-types-data-*/
**/vendor/bundle/ruby/*/gems/mime-types-*/
**/vendor/bundle/ruby/*/gems/mini_portile2-*/
Expand Down
1 change: 0 additions & 1 deletion Library/Homebrew/Gemfile
View file
Open in desktop
Expand Up @@ -15,7 +15,6 @@ end
gem "bootsnap", require: false
gem "byebug", require: false
gem "json_schemer", require: false
gem "method_source", require: false
gem "minitest", require: false
gem "parallel_tests", require: false
gem "ronn", require: false
Expand Down
1 change: 0 additions & 1 deletion Library/Homebrew/Gemfile.lock
View file
Open in desktop
Expand Up @@ -233,7 +233,6 @@ DEPENDENCIES
did_you_mean
json_schemer
mechanize
method_source
minitest
parallel_tests
parlour
Expand Down
6 changes: 0 additions & 6 deletions Library/Homebrew/cask/artifact/abstract_flight_block.rb
View file
Open in desktop
Expand Up @@ -36,12 +36,6 @@ def summarize
directives.keys.map(&:to_s).join(", ")
end

def to_h
require "method_source"

directives.transform_values(&:source)
end

private

def class_for_dsl_key(dsl_key)
Expand Down
3 changes: 2 additions & 1 deletion Library/Homebrew/cask/cask.rb
View file
Open in desktop
Expand Up @@ -313,7 +313,8 @@ def artifacts_list
artifacts.map do |artifact|
case artifact
when Artifact::AbstractFlightBlock
artifact.to_h
# Only indicate whether this block is used as we don't load it from the API
Copy link
Member

Choose a reason for hiding this comment

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

Could do similarly for languages

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 think that including the languages in the JSON still is useful for users even if we don't use it.

Copy link
Member

Choose a reason for hiding this comment

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

@apainintheneck Given no users ever requested it and it increases the size of the JSON: I disagree.

{ artifact.summarize => nil }
when Artifact::Relocated
# Don't replace the Homebrew prefix in the source path since the source could include /usr/local
source, *args = artifact.to_args
Expand Down
6 changes: 1 addition & 5 deletions Library/Homebrew/cask/cask_loader.rb
View file
Open in desktop
Expand Up @@ -301,11 +301,7 @@ def load(config:)

json_cask[:artifacts].each do |artifact|
key = artifact.keys.first
if FLIGHT_STANZAS.include?(key)
instance_eval(artifact[key])
else
send(key, *artifact[key])
end
send(key, *artifact[key])
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
send(key, *artifact[key])
send(key, *artifact[key]) if artifact[key].present?

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would that be needed? You can't reach this code if you use a flight stanza.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You shouldn't make it past the following code if you have a flight artifact.

brew/Library/Homebrew/cask/cask_loader.rb

Lines 233 to 238 in 2a417f1

# Use the cask-source API if there are any `*flight` blocks or the cask has multiple languages
if json_cask[:artifacts].any? { |artifact| FLIGHT_STANZAS.include?(artifact.keys.first) } ||
json_cask[:languages].any?
cask_source = Homebrew::API::Cask.fetch_source(token, git_head: json_cask[:tap_git_head])
return FromContentLoader.new(cask_source).load(config: config)
end

end

caveats json_cask[:caveats] if json_cask[:caveats].present?
Expand Down
48 changes: 0 additions & 48 deletions Library/Homebrew/test/cask/cask_spec.rb
View file
Open in desktop
Expand Up @@ -265,46 +265,6 @@
}
JSON
}
let(:expected_flight_variations) {
<<~JSON
{
"arm64_big_sur": {
"artifacts": [
{
"preflight": " preflight do\\n puts \\"preflight on Big Sur\\"\\n end\\n"
},
{
"uninstall_postflight": " uninstall_postflight do\\n puts \\"uninstall_postflight on Big Sur\\"\\n end\\n"
}
]
},
"big_sur": {
"artifacts": [
{
"preflight": " preflight do\\n puts \\"preflight on Big Sur\\"\\n end\\n"
},
{
"uninstall_postflight": " uninstall_postflight do\\n puts \\"uninstall_postflight on Big Sur\\"\\n end\\n"
}
]
},
"catalina": {
"artifacts": [
{
"preflight": " preflight do\\n puts \\"preflight on Catalina or older\\"\\n end\\n"
}
]
},
"mojave": {
"artifacts": [
{
"preflight": " preflight do\\n puts \\"preflight on Catalina or older\\"\\n end\\n"
}
]
}
}
JSON
}

before do
# Use a more limited symbols list to shorten the variations hash
Expand Down Expand Up @@ -340,13 +300,5 @@
expect(h).to be_a(Hash)
expect(JSON.pretty_generate(h["variations"])).to eq expected_sha256_variations.strip
end

it "returns the correct variations hash for a cask with conditional flight blocks" do
c = Cask::CaskLoader.load("conditional-flight")
h = c.to_hash_with_variations

expect(h).to be_a(Hash)
expect(JSON.pretty_generate(h["variations"])).to eq expected_flight_variations.strip
end
end
end
21 changes: 0 additions & 21 deletions Library/Homebrew/test/support/fixtures/cask/Casks/conditional-flight.rb
View file
Open in desktop

This file was deleted.

141 changes: 0 additions & 141 deletions Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/method_source-1.0.0/lib/method_source.rb
View file
Open in desktop

This file was deleted.