search

mroonga / homebrew-mroonga

The homebrew formula for Mroonga.
7 stars 13 forks source link

Modernize Mroonga formula #3

Closed cosmo0920 closed 9 years ago

cosmo0920 commented 9 years ago

そのままmasterに入れて良いか判断つかなかったのでpull requestにします。

要約

$ brew audit mroonga

を行うと以下のようなErrorが出るようになっていました。

mroonga/mroonga/mroonga:
 * "Formula.factory(name)" is deprecated in favor of "Formula[name]"
 * Use build instead of ARGV to check options
 * Use build instead of ARGV to check options
 * Use new-style option definitions
 * Use build instead of ARGV to check options
 * Use build instead of ARGV to check options
 * Use new-style test definitions (test do)
 * Use build instead of ARGV to check options

Error: 8 problems in 1 formula

Pull Request の作業内容

変更点

最後のエラー * Use build instead of ARGV to check options は option_value で使われているため解消する方法が思いつきませんでした…。

解決済みhttps://github.com/mroonga/homebrew-mroonga/commit/16f95ac1e978d73fa9f775c2e667e70c2a2e4d76

動作確認

上の二つに加えて以下も足した

のオプションでMroongaが正常にインストール出来るのを確認しました。

kou commented 9 years ago

おぉ、pull requestを読む人が必要な情報がちゃんと入っているいいpull requestですね！ 動作確認もしていて感慨深いです。

合わせて --use-xxx のスタイルのオプションを --with-xxx に変更しました。

これは--use-xxxじゃない方がいいんですか？他の--with-xxxはON/OFFとか値を指定するためのもので、--use-xxxは排他的にどれかを選択するためのもので、役割が違うものなんですが、--with-xxxにした方がよいのでしょうか。

最後のエラー * Use build instead of ARGV to check options は option_value で使われているため解消する方法が思いつきませんでした…。

build.used_options.eachにすると同じ挙動にならないですかねぃ。

cosmo0920 commented 9 years ago

合わせて --use-xxx のスタイルのオプションを --with-xxx に変更しました。

これは--use-xxxじゃない方がいいんですか？他の--with-xxxはON/OFFとか値を指定するためのもので、--use-xxxは排他的にどれかを選択するためのもので、役割が違うものなんですが、--with-xxxにした方がよいのでしょうか。

--use-xxx のオプションの使う方法に慣れていなかったのでうっかり自分の知っている方向に寄せてしまっていました。意味の違うオプションなら元のオプションに戻して、今風のHomebrewスタイルへ書き直す方が良いですね。と言う事で --use-xxx オプションを使いつつ今風のスタイルに書き換えました。

最後のエラー * Use build instead of ARGV to check options は option_value で使われているため解消する方法が思いつきませんでした…。

build.used_options.eachにすると同じ挙動にならないですかねぃ。

残念ながらなりませんでした。。

/usr/local/Library/Taps/mroonga/homebrew-mroonga/mroonga.rb:166:in `block in option_value'
/System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/lib/ruby/2.0.0/set.rb:232:in `each_key'
/System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/lib/ruby/2.0.0/set.rb:232:in `each'
/usr/local/Library/Homebrew/options.rb:69:in `each'
/usr/local/Library/Taps/mroonga/homebrew-mroonga/mroonga.rb:165:in `option_value'
/usr/local/Library/Taps/mroonga/homebrew-mroonga/mroonga.rb:115:in `build_configure_args'
/usr/local/Library/Taps/mroonga/homebrew-mroonga/mroonga.rb:144:in `install_mroonga'
/usr/local/Library/Taps/mroonga/homebrew-mroonga/mroonga.rb:48:in `block (2 levels) in install'
/usr/local/Library/Taps/mroonga/homebrew-mroonga/mroonga.rb:47:in `chdir'
/usr/local/Library/Taps/mroonga/homebrew-mroonga/mroonga.rb:47:in `block in install'
/usr/local/Library/Taps/mroonga/homebrew-mroonga/mroonga.rb:105:in `block in build_formula'
/usr/local/Library/Homebrew/formula.rb:540:in `block in brew'
/usr/local/Library/Homebrew/formula.rb:935:in `block in stage'
/usr/local/Library/Homebrew/resource.rb:91:in `block in unpack'
/usr/local/Library/Homebrew/extend/fileutils.rb:17:in `mktemp'
/usr/local/Library/Homebrew/resource.rb:88:in `unpack'
/usr/local/Library/Homebrew/resource.rb:81:in `stage'
/usr/local/Library/Homebrew/formula.rb:933:in `stage'
/usr/local/Library/Homebrew/formula.rb:536:in `brew'
/usr/local/Library/Taps/mroonga/homebrew-mroonga/mroonga.rb:104:in `build_formula'
/usr/local/Library/Taps/mroonga/homebrew-mroonga/mroonga.rb:46:in `install'
/usr/local/Library/Homebrew/build.rb:129:in `block in install'
/usr/local/Library/Homebrew/formula.rb:540:in `block in brew'
/usr/local/Library/Homebrew/formula.rb:935:in `block in stage'
/usr/local/Library/Homebrew/resource.rb:91:in `block in unpack'
/usr/local/Library/Homebrew/extend/fileutils.rb:17:in `mktemp'
/usr/local/Library/Homebrew/resource.rb:88:in `unpack'
/usr/local/Library/Homebrew/resource.rb:81:in `stage'
/usr/local/Library/Homebrew/formula.rb:933:in `stage'
/usr/local/Library/Homebrew/formula.rb:536:in `brew'
/usr/local/Library/Homebrew/build.rb:107:in `install'
/usr/local/Library/Homebrew/build.rb:179:in `<main>'
kou commented 9 years ago

--use-xxx のオプションの使う方法に慣れていなかったのでうっかり自分の知っている方向に寄せてしまっていました。意味の違うオプションなら元のオプションに戻して、今風のHomebrewスタイルへ書き直す方が良いですね。と言う事で --use-xxx オプションを使いつつ今風のスタイルに書き換えました。

検討・修正ありがとうございます。

残念ながらなりませんでした。。

このバックトレースの1行上にエラーメッセージがあるような気がするんですが、確認してもらえませんか？

cosmo0920 commented 9 years ago

おっと、すみません

$ brew install mroonga --use-homebrew-mariadb
<snip>
Error: undefined method `split' for #<Option: "--use-homebrew-mariadb">
Please report this bug:
    https://git.io/brew-troubleshooting

ですね。

kou commented 9 years ago

Optionならto_sしてからsplitで動かないかしら。

cosmo0920 commented 9 years ago

Optionならto_sしてからsplitで動かないかしら。

おー。動きました！！

cosmo0920 commented 9 years ago

メモ：現在のHomebrewのmasterではbrew audit [Formula].rb よりも更に強力にシンタックスをチェックする brew audit --strict [Formula].rb がJenkins CIに仕掛けられている。そのため、もしもHomebrewに取り込んでもらうならこちらの警告も除去しないといけない。

今回は brew audit への対応ということで、未解決のエラーはなくなっています。そのため、本Pull Requestでは警告の除去はここまで、です。

cosmo0920 commented 9 years ago

ちなみに↑の brew audit --strict [Formula] を実行すると見事に --use-xxx なものは警告に引っかかる

んですが、ちょっとこれは流石にきつ過ぎる変更かもしれないのでどうにかしたいですね…

kou commented 9 years ago

マージしました！

Homebrewに取り込んでもらうなら

昔、Homebrewの人に聞いたら難しすぎるって言われた https://github.com/Homebrew/homebrew/pull/11999 のでムリじゃないかなぁと思っています。

ちなみに↑の brew audit --strict [Formula] を実行すると見事に --use-xxx なものは警告に引っかかる

Homebrewがそういう方針なら--with-xxxにした方がいいかもしれないですねぇ。

cosmo0920 commented 9 years ago

マージありがとうございます。

Homebrewに取り込んでもらうなら

昔、Homebrewの人に聞いたら難しすぎるって言われた Homebrew/homebrew#11999 のでムリじゃないかなぁと思っています。

うーん、そうですかぁ。

実はPGroongaのプルリクエスト https://github.com/Homebrew/homebrew/pull/41174 も不穏な空気が漂っています。。。 PGroongaのリポジトリでもface to faceでもいいので相談させてください！