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

Vendor public_suffix data dir #13301

Merged
merged 1 commit into from May 19, 2022

Conversation

samford
Copy link
Member

@samford samford commented May 19, 2022

  • 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 is a quick follow-up to #13300 to fix another vendored gem bug. This one only appeared when running brew livecheck on a formula with a using: :homebrew_curl URL (e.g., kim-api), which exercises the addressable code in Livecheck#use_homebrew_curl?:

No such file or directory @ rb_sysopen - /opt/homebrew/Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/public_suffix-4.0.7/data/list.txt
/opt/homebrew/Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/public_suffix-4.0.7/lib/public_suffix/list.rb:51:in `read'
/opt/homebrew/Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/public_suffix-4.0.7/lib/public_suffix/list.rb:51:in `default'
/opt/homebrew/Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/public_suffix-4.0.7/lib/public_suffix.rb:67:in `parse'
/opt/homebrew/Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/public_suffix-4.0.7/lib/public_suffix.rb:141:in `domain'
/opt/homebrew/Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/addressable-2.8.0/lib/addressable/uri.rb:1234:in `domain'
/opt/homebrew/Library/Homebrew/livecheck/livecheck.rb:538:in `use_homebrew_curl?'
/opt/homebrew/Library/Homebrew/livecheck/livecheck.rb:671:in `block in latest_version'
/opt/homebrew/Library/Homebrew/livecheck/livecheck.rb:622:in `each'
/opt/homebrew/Library/Homebrew/livecheck/livecheck.rb:622:in `each_with_index'
/opt/homebrew/Library/Homebrew/livecheck/livecheck.rb:622:in `latest_version'
/opt/homebrew/Library/Homebrew/livecheck/livecheck.rb:276:in `block in run_checks'
/opt/homebrew/Library/Homebrew/livecheck/livecheck.rb:216:in `map'
/opt/homebrew/Library/Homebrew/livecheck/livecheck.rb:216:in `with_index'
/opt/homebrew/Library/Homebrew/livecheck/livecheck.rb:216:in `run_checks'
/opt/homebrew/Library/Homebrew/dev-cmd/livecheck.rb:115:in `livecheck'
/opt/homebrew/Library/Homebrew/brew.rb:110:in `<main>'

The addressable gem depends on the public_suffix gem and we also need to vendor the data dir from public_suffix for addressable to function correctly. This PR updates .gitignore and checks in the public_suffix-*/data directory.

Since public_suffix is only a dependency of addressable (we're not using is directly), do we need to add it to the Gemfile or is this fine as-is?

The `addressable` gem depends on the `public_suffix` gem but we also
need to vendor the `data` dir from `public_suffix` for `addressable`
to function correctly.
@samford samford added the critical Critical change which should be shipped as soon as possible. label May 19, 2022
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

@Bo98
Copy link
Member

Bo98 commented May 19, 2022

Since public_suffix is only a dependency of addressable (we're not using is directly), do we need to add it to the Gemfile or is this fine as-is?

This is fine.

@samford samford enabled auto-merge May 19, 2022 01:04
@samford samford merged commit 5d4353f into Homebrew:master May 19, 2022
@samford
Copy link
Member Author

samford commented May 19, 2022

Thanks for the quick review, Bo!

This should be the last of the vendoring with respect to the addressable addition. At least I have a better idea of how to handle this type of situation in the future.

@samford samford deleted the add-public_suffix-data-dir branch May 19, 2022 01:30
@github-actions github-actions bot added the outdated PR was locked due to age label Jun 19, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants