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

appveyorでロケール設定するのをやめる #482

Merged

Conversation

berryzplus
Copy link
Contributor

目的

appveyorのビルドを速くする

概要

appveyorのビルドに時間がかかる要因として、
ビルド前にシステムロケールを変更して再起動を行っていることをあげられます。
ロケール変更に必要な時間は、混雑具合などにもよりますが2分くらいです。

何故ロケール変更をしているかというと、
サクラエディタが日本語アプリだからです。

もう少し厳密な言い方をすると、
日本語で書かれているソースコードをコンパイルするためには、
システムロケールを変える必要があったためです。
(日本語で書かれている=文字コードがSJISだと思ってください。)

#112 (comment) で書いた通り、
このロケール設定をしなくて済む方法がありそうです。

修正内容の説明

この修正は code&fix で検証しながら作ったものなので、やったことを書きます。

  1. 単純にロケール設定を外すだけの修正をしてみた
     →ビルドエラーになった。
  2. HeaderMakeに異常がありそうなので調べてみた
     →Funccode_x.hsrcをcl.exeでプリプロセスするときに警告が出てることが分かった
  3. Funccode_x.hsrcの文字コードがSJISであることに気付いた
     →Funccode_x.hsrcの文字コードを変換してみた
  4. cl.exeのプリプロセスで警告がでるようになった
     →cl.exeのコマンドラインに/utf-8を足してみた
  5. cl.exeのプリプロセスの警告が消えた
     →本体側のビルドで警告が消えないことに気付いた
  6. sakura.vcxprojのadditional optionに文字コード指定(sjis)があることに気付いた
     →additional optionを/utf-8に差し替えた
  7. 警告全部消えた

確認内容

ソースコードがすべてUNICODE化されたため、
英語環境でも問題なくビルドできるようになっているはずです。

ロケール変更をせずにビルドしています。

「asmに差が出ないこと」でチェックできるかも知れません。
生成されたexeはいちおう日本語表示できてそうです。

プロジェクト設定のSJIS指定をutf-8指定に変更する
Funccode_x.hsrcをUTF-8にする
HeaderMake.cppの文字コード指定を更新する
Makefileの文字コード指定を更新する
HeaderMakeのコピペと考えられる
紛らわしいので削っておく
@k-takata
Copy link
Member

最近、日本語ロケールに依存したバッチファイルを追加していませんでしたっけ?
%DATE%の結果を読むのか何か忘れましたが。

@m-tmatma
Copy link
Member

最近、日本語ロケールに依存したバッチファイルを追加していませんでしたっけ?
%DATE%の結果を読むのか何か忘れましたが。

それは get-PR.bat なので、CI 上では動きません。

@m-tmatma
Copy link
Member

ASM ファイルはいろいろ差分が出ています。

@m-tmatma
Copy link
Member

古い方は 36883af を使いましたが、
36883af がなんかおかしい。
アセンブラのコード部分が欠落しているように思う。

@beru beru added CI appveyor など CI 関連 【ChangeLog除外】 🚅 speed up 🚀 高速化 labels Sep 25, 2018
@berryzplus
Copy link
Contributor Author

古い方は 36883af を使いましたが、
36883af がなんかおかしい。
アセンブラのコード部分が欠落しているように思う。

確認ありがとうございます。
何をもってokとするか微妙なので検証ありがたいです。どう違ってるかによって出ていい差分なのかどうか変わると思うので後でぼくもチェックしてみます。

@berryzplus
Copy link
Contributor Author

アセンブラに差が出る理屈が分かったようなので追加コミット積みました。
https://msdn.microsoft.com/ja-jp/library/mt708821.aspx

/utf-8というオプションを使って文字コード指定したのですが、これはソースコードと実行バイナリに埋め込まれる文字コードの両方を指定するオプションらしいです。

将来的に /utf-8 を使う話もあるかもしれませんが、このPRのスコープでそこまでやりたいとは思っていませんでした。

ビルド終わったら確認して声かけます。

@berryzplus
Copy link
Contributor Author

ビルド終わりました。

CDlgAbout.asm以外のasmファイルが一致することを確認できました。
このファイルのasmに差分が出るのは githash.h でdefineした情報から表示用の固定文字列を組み立てて保持している都合です。

ちょっと回り道したけれど、なんとか妥当性が確認できた感じです。
あらためてレビューをお願いします m( . . ) m

@m-tmatma
Copy link
Member

CDlgAbout.asm以外のasmファイルが一致することを確認できました。

比較したビルドの URL を共有していただけますか?

古い方は 36883af を使いましたが、
36883af がなんかおかしい。
アセンブラのコード部分が欠落しているように思う。

これはローカルで試しましたが、うまく動いていないのかもしれない。
Visual studio 2017 の 15.8.5 を使っています。

@berryzplus
Copy link
Contributor Author

@berryzplus
Copy link
Contributor Author

Visual studio 2017 の 15.8.5 を使っています。

バージョンはうちもあげてて、ビルド後のexeは動いてそうに見えます。

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.

対応ありがとうございます。

@m-tmatma m-tmatma added this to the next release milestone Sep 25, 2018
@berryzplus berryzplus merged commit ec23dc7 into sakura-editor:master Sep 25, 2018
@berryzplus berryzplus deleted the feature/sourcecode_globalization branch September 25, 2018 13:55
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019
…de_globalization

appveyorでロケール設定するのをやめる
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚅 speed up 🚀 高速化 CI appveyor など CI 関連 【ChangeLog除外】
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants