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

Makefile調整:入力ファイルUTF-8指定、GitHash出力 #306

Closed
wants to merge 4 commits into from

Conversation

kobake
Copy link
Member

@kobake kobake commented Jul 26, 2018

以下の調整を行いました。

  • 入力ファイルのエンコーディング指定が cp932 になっていたのを utf-8 に変更
  • svnrev 出力処理を githash 出力処理に差し替え

注記

自分の手元ではビルド全成功はしません(auto イテレータのところでコンパイルエラーになる。自分の gcc が悪いだけかも。)が、対応前よりはビルド進むようになった感じです。

@kobake kobake changed the title Makefile調整:入力ファイルUTF-8、GitHash出力 Makefile調整:入力ファイルUTF-8指定、GitHash出力 Jul 26, 2018
Copy link
Member

@m-tmatma m-tmatma left a comment

Choose a reason for hiding this comment

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

git hash の生成は vc と共通化した方がいいです。

@kobake
Copy link
Member Author

kobake commented Jul 26, 2018

git hash の生成は vc と共通化した方がいいです。

それはそうなのですが、今のところは暫定対処ということで考えています。
preBuild.bat って Makefile から呼ばれることも考えられて作られてます?

@@ -32,7 +32,7 @@ DEFINES= \
-DUNICODE \
-DNDEBUG
CFLAGS= -O2 \
-finput-charset=cp932 -fexec-charset=cp932 \
-finput-charset=utf-8 -fexec-charset=utf-8 \
Copy link
Member

Choose a reason for hiding this comment

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

-fexec-charsetcp932 ではないでしょうか。
utf-8 にすると #265 (comment) で指摘した問題が出ると思います。

Copy link
Member Author

@kobake kobake Jul 26, 2018

Choose a reason for hiding this comment

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

なるほど。 e8afea7 で cp932 に戻しました。

@berryzplus
Copy link
Contributor

さすが・・・

って、あれ?

なんでmakefileまだ保守してるんでしたっけ?
自動生成ファイルだから消しませんか?って話の結末がどうなったか曖昧です・・・

@m-tmatma
Copy link
Member

なんでmakefileまだ保守してるんでしたっけ?

#44 (comment)

@m-tmatma
Copy link
Member

preBuild.bat って Makefile から呼ばれることも考えられて作られてます?

考慮はされてないですが、
githash.bat とかいう名前で分離したらいいと思います。

@m-tmatma
Copy link
Member

まあ、別 PR でいいですね。

@kobake
Copy link
Member Author

kobake commented Jul 26, 2018

はい、エンコーディングを変更した btool のビルド確認ができれば良いかなくらいの気持ちで今回の対応してます

@berryzplus
Copy link
Contributor

berryzplus commented Jul 26, 2018

@k-takata さんにご指摘いただいた件ですが、対応が必要だと思います。

↓ btool フォルダに入っている gcc 向けツールをビルドしている。

sakura/sakura_core/Makefile

Lines 445 to 446 in 611dcc6

$(RCTOOL): $(RCTOOLDIR)/mrc2grc.cpp
$(CXX) $(CXXFLAGS) $(RCTOOLDIR)/mrc2grc.cpp -o $@ -static-libgcc

↓ビルドしたツールを使ってshiftJISの ダメ文字 の後に '\' を追加し、windresを使って *.rc をビルドし、一時ファイルを消している

sakura/sakura_core/Makefile

Lines 451 to 454 in 611dcc6

.rc.o:
$(RCTOOL) $< sakura_grc.rc
$(RC) --language=0411 $(DEFINES) sakura_grc.rc -o $@
$(RM) sakura_grc.rc

ダメ文字とは、マルチバイト文字の 2byte目 に 0x5c (バックスラッシュ) が来る文字のこと。
ここに例が載ってます → ダメ文字

UTF-8の場合、2byte目以降は必ず最上位ビットが立つので、
2byte目に 0x5c が来る状況は発生しません。 → UTF-8 Wikipedia
なので、今後 btool/mrc2grc.cpp は不要になります。

一方、windresに対してリソーススクリプトの文字コードを教えてあげないといけないので、その分の修正が必要です。
452と454を削って、453に -c 65001 追加です。

手元環境にあった windres(binutil-2.27) には -c 65001 を受け付けました。
去年末くらいに落としてきた pleiades 最新版についてた mingw-w64 のものです。

Makefileを消しちゃダメな理由・・・

MakefileMake の挙動は「既存の Makefile を読み取って書き換える」処理になっているので、単に Makefile を消してしまうとうまく動作しません。

了解です。すっかり忘れておりました。

@m-tmatma
Copy link
Member

最終的には appveyor ビルドしたいです。
https://www.appveyor.com/docs/build-environment/#mingw-msys-cygwin

@berryzplus
Copy link
Contributor

このPRは今、451行目あたりのRC(windres)オプションに変更が必要で止まってる認識・・・。

@kobake
Copy link
Member Author

kobake commented Aug 7, 2018

ご無沙汰しております。
遅くなりましたが以下の対応を行いました。

@berryzplus

一方、windresに対してリソーススクリプトの文字コードを教えてあげないといけないので、その分の修正が必要です。
452と454を削って、453に -c 65001 追加です。

結果、btool は不要になったっぽい気がしますが、それについてどうするかの対応は別件として考えたいです。

@@ -50,380 +50,19 @@ exe= sakura.exe
OBJS= \
CAutoReloadAgent.o \
CAutoSaveAgent.o \
CBackupAgent.o \
CCodeChecker.o \
Copy link
Member

Choose a reason for hiding this comment

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

ここら辺ばっさり削っていいのですか?

Copy link
Member Author

Choose a reason for hiding this comment

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

あ、すみません、実験用コードが混じってました。直します

Copy link
Member Author

Choose a reason for hiding this comment

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

しょうもなすぎるミスコミットだったので修正コミット上書いて push -f しました

@kobake
Copy link
Member Author

kobake commented Aug 7, 2018

夜にまた見に来ます

@m-tmatma
Copy link
Member

m-tmatma commented Aug 7, 2018

ビルド手順を共有してほしいです。
(できれば markdown で)

@kobake
Copy link
Member Author

kobake commented Aug 7, 2018

cd sakura_core
make

です。自分も詳細は把握しておらず手探りです。

@berryzplus
Copy link
Contributor

メイクコマンドはmingw32–makeでやるものらしいです。こいつはpathにshがいると誤動作するのでgit
にpath を通していたら外してやる必要があります。

@berryzplus
Copy link
Contributor

berryzplus commented Aug 8, 2018

追加情報

結論、いまMinGWではビルドできなくなっています。
最近入れたいくつかの修正が原因です。

確認のための情報共有

確認には pleiades (Eclipse日本語版) に同梱されている MinGW64 を使っています。
http://mergedoc.osdn.jp/

とりあえず、ソースを直しながら sakura.exe がビルドされるところまで確認はしました。
ぼくが確認に使ったのは 4.6 Neon です。バージョンはGNU 6.2.0。これより前だとnoexceptがエラーになった(C++11未対応?)ので、これを使いました。

最終的に修正したファイル→ mingw_fix_files.zip
対応方法は色々あると思いますので、最低限「ビルドできる状態にするには?」のサンプルだと思ってください。

ビルド確認手順

Windowsでのやり方

  1. テキトーな場所でコマンドプロンプトを開く
  2. sh と叩いて bash やら sh が起動しないことを確認する
  3. sh(bash) が起動してしまった場合、pathを設定しなおしてpath上にshがいない状態にする
  4. MinGWのbinフォルダをpathに入れる
path=C:\eclipse4.6\eclipse\mingw\bin;%path%
  1. sakuraのgit-cloneがあるフォルダに移動してビルド
cd C:\gitroot\sakura\sakura_core
mingw32-make

ビルドには超時間かかります。20分くらいかかってるはず。

以上

@m-tmatma
Copy link
Member

m-tmatma commented Aug 9, 2018

ビルドには超時間かかります。20分くらいかかってるはず。

-jN オプションを使うと速くなりますか?

@kobake
Copy link
Member Author

kobake commented Aug 11, 2018

補足しておくとこの PR 一発で Makefile を完璧なものに仕上げるつもりはなくて、
最低限 #304 の btool ビルドが確認できるようにだけはしておきたい、というのがもともとの目的です

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

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

start reviewのまま放置していたコメントがあったので入れときます。

$(RCTOOL) $< sakura_grc.rc
$(RC) --language=0411 $(DEFINES) sakura_grc.rc -o $@
$(RM) sakura_grc.rc
$(RC) -c65001 --language=0411 $(DEFINES) sakura_rc.rc -o $@
Copy link
Contributor

Choose a reason for hiding this comment

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

すみません。提示した修正案が間違っておりました。

-c UTF8と書かないと動かないみたいです。

Copy link
Member Author

Choose a reason for hiding this comment

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

なんだか自分の環境では -c65001 でも -c UTF8 でも、もしくは -c オプション入れなくても sakura_rc.o の内容は全部同じになりました。環境依存ありますかね?

Copy link
Contributor

Choose a reason for hiding this comment

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

-c65001 でも -c UTF8 でも

ん?-c65001-c 65001じゃなくて?
・・・コマンドラインの書き方間違ってた気配がw

Copy link
Member Author

Choose a reason for hiding this comment

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

-c65001-c 65001 もどっちもイケて、どっちも同じ結果でした

@@ -32,7 +32,7 @@ DEFINES= \
-DUNICODE \
-DNDEBUG
CFLAGS= -O2 \
-finput-charset=cp932 -fexec-charset=cp932 \
-finput-charset=utf-8 -fexec-charset=cp932 \
Copy link
Contributor

Choose a reason for hiding this comment

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

ちゃんと試せてませんが「-finput-charset=utf-8」はなしでもいけてます。
「-finput-charset=utf8」だと「iconvにそんなエンコード名ないよ」と怒られます。

Copy link
Member Author

Choose a reason for hiding this comment

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

まぁもともと明示的な指定があったみたいなのでそれを踏襲して utf-8 にした、という経緯があります。無くても BOM 付き UTF-8 なら自動判定してくれそうではありますね。

Copy link
Contributor

Choose a reason for hiding this comment

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

問題ないっす。
-finput-charset=utf-8 を付けた場合で問題ないことも確認できてます。

@kobake
Copy link
Member Author

kobake commented Aug 12, 2018

何度か言及していますがこの PR は #304 の btool ビルドを最低限チェックできるようにするためのものです。この PR だけで完璧な Makefile を作り上げる目的ではないことをご理解ください。

@berryzplus
Copy link
Contributor

追加情報

ぼくが試した mingw32-make を使う方法だと git に PATH を通せないので、githashの生成がうまくいきません。MinGWでのビルドをサポートしている他のプロジェクト、たとえばgoogletestの場合、appveyor側で PATH からGITを外している模様。

あと、生成された sakura.exe は起動できないっぽいw

@kobake
Copy link
Member Author

kobake commented Aug 12, 2018

ま~、現状使ってる人が(おそらく)いない Makefile なので、そんなものでしょう。
むしろ sakura.exe 生成まで成功していることに驚きw

ただ、btool のビルドを確かめるためには現状では Makefile 必要なのでこの PR 上げた感じです。btool 自体が廃止される気配濃厚ですが。

@@ -449,14 +453,12 @@ $(HEADERMAKE): $(HEADERMAKETOOLDIR)/HeaderMake.cpp
$(CXX) $(CXXFLAGS) $(HEADERMAKETOOLDIR)/HeaderMake.cpp -o $@ -static-libgcc

.rc.o:
Copy link
Member

Choose a reason for hiding this comment

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

ここ間違ってませんか?
rc ファイルのコンパイル結果は res ファイルです。

Copy link
Contributor

Choose a reason for hiding this comment

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

ここは使う人が勝手に決めるものなので、間違ってるとかではないのかな、と思います。
拡張子 res は microsoft が勝手に決めた仕様ですが、これはwindowsの仕様じゃないです。
ちなみに cmakeが吐く Makefile だと .rc.obj になります。
というわけで、rc→oで問題ないんじゃないかと思います。

Copy link
Member

Choose a reason for hiding this comment

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

MinGWではリソースファイルはCOFF obj形式にコンパイルして使います。
res形式はリンクできなかったような?

Copy link
Member

Choose a reason for hiding this comment

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

そうなんや

@berryzplus
Copy link
Contributor

これのステータスってどうなんでしたっけ?

  • このPRの当初の目的(btoolのビルド確認)は失われている
  • Makefileの修正だけではビルドが通らないことが分かっている Makefile調整:入力ファイルUTF-8指定、GitHash出力 #306 (comment)
  • このあと CMakeLists.txt を完成させて「appveyor で MinGW ビルドする」という対応が控えているので生Makefileを修正する意味はあんまりなくなっている

@kobake
Copy link
Member Author

kobake commented Aug 15, 2018

これのステータスってどうなんでしたっけ?

初めから完璧な Makefile を仕上げる意図はなく、btool のビルド確認およびそれに付随して githash の対応を行うためだけの対応でした。

些細な目的と変更なので、まさかこの PR で理想状態を求められるとは思いませんでしたので、もう疲れてますし、正直どうでもいいかな、と思ってます。
他の人のほうで勝手にどうするか決めてください。クローズしても良いです。

コントリビュータに過度の負担をかける運用は将来必ず破綻することを常に頭に置いてください。

@berryzplus
Copy link
Contributor

了解です。これと重複するPRを投げてよいものか判断つかなかったのでどうしたいか確認したかったです。(このPRに継ぎ足しでやるか、別目的で新たにPR起こすか。)

単純にビルドできる状態にもっていく試行は一度しているので、折をみて別PRを起こす腹積もりでいます。 #306 (comment)

@kobake
Copy link
Member Author

kobake commented Aug 19, 2018

res 出力を完璧にすることはこの PR の目的ではないことを再度書いておきます。そういうのはこの PR 対応の後で追加でやってください。現状の master がそもそも不完全なものであり、この PR がその不完全性をさらに上げる等でない限りはマージすることに問題は無いと考えています。

この PR がマージされたら #304 をマージする動きにします。

そもそも先に btool 消す対応するなら #306#304 もマージせずにクローズします。

選んでください。

@berryzplus
Copy link
Contributor

この PR の目的は btool のビルド確認だと認識しています。
btool は廃止の方向で認識があったと思っています。
なので、この PR は目的を失ったと判断しております。
マージしてよいかの判断基準「目的を達成しているか」を満たせなくなったと理解しております。

そもそも先に btool 消す対応するなら #306#304 もマージせずにクローズします。

#351 がマージされたら btool 消す対応を進める腹積もりです。

@kobake
Copy link
Member Author

kobake commented Aug 19, 2018

ちょっとそういうどっちつかずの答えはダメですよ。クローズします。

@kobake kobake closed this Aug 19, 2018
@kobake kobake deleted the makefile-utf8-githash branch August 19, 2018 07:46
@m-tmatma m-tmatma added the Won't Fix 対応しない【ChangeLog除外】 label Aug 25, 2018
@ds14050 ds14050 added the Won't Fix 対応しない【ChangeLog除外】 label Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Won't Fix 対応しない【ChangeLog除外】
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants