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

SVN Revision 利用箇所を削除し、代わりに GitHash を用いる #147

Merged
merged 2 commits into from
Jun 20, 2018
Merged

SVN Revision 利用箇所を削除し、代わりに GitHash を用いる #147

merged 2 commits into from
Jun 20, 2018

Conversation

kobake
Copy link
Member

@kobake kobake commented Jun 19, 2018 •
edited
Loading

SVN Revision を利用しているコードは全部削除したつもりでいましたが、まだ一部残っていたようなので、不要なところは削除、および必要に応じて代わりに GitHash に差し替える対応をしました。

対応前の不具合

ソースコード一式を git clone ではなく zip でダウンロードした場合に SVN Revision の依存が残っているせいでビルドが失敗するという報告をいただいていました。

https://twitter.com/arigayas/status/1009162151934623744

@arigayas
興味本位で #サクラエディタ GitHub からソースコードのzipファイルでダウンロードしてビルドしてみたけど、失敗する。
しかしC++の知識が皆無なので対処出来ない(苦笑)

C2146 構文エラー: ')' が、識別子 'LSVN_REV_STR' の前に必要です。 \sakura_core\dlg\cdlgabout.cpp 182

自分の環境でも試してみましたが、確かに同様のエラーが発生していました。

本PRの対応によりこのエラーは解消されます。

m-tmatma
m-tmatma requested changes Jun 19, 2018
View reviewed changes
sakura_core/dlg/CDlgAbout.cpp
@@ -173,13 +173,6 @@ BOOL CDlgAbout::OnInitDialog( HWND hwndDlg, WPARAM wParam, LPARAM lParam )
HIWORD(dwVersionLS),
LOWORD(dwVersionLS)
);
Copy link
Member

@m-tmatma m-tmatma Jun 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

GIT_COMMIT_HASH が定義されていないとバージョン情報自体がなくなります。
GIT_COMMIT_HASH が定義されていないとき GIT_COMMIT_HASH の以外の部分は残すべきだと思います。

Copy link
Member Author

Choose a reason for hiding this comment

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

本PRはあくまでも SVN 依存のせいでビルド自体が通らなくなることの解決を目的としているので、別件対応とさせてください。#148 を立てました。

ちなみにここのロジックは #19 で対応されたところですね。（当時の時点では git hash が存在しないケースが考慮されていなかったと思われる）

Copy link
Member

Choose a reason for hiding this comment

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

指摘した箇所は この PR で修正されたもので過去の対応ではないです。
GitHub 上では見にくいですが、ローカルで見るとよく分かると思います。
他にあるかは確認していないですが。

Copy link
Member

@m-tmatma m-tmatma Jun 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

↑ 間違いです。
@kobake さんの
#147 (comment) が正しいです。

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.

朝からお疲れ様です 🙏

@m-tmatma
Copy link
Member

#90 で同じところ修正しているので、#90 はこの PR を取り込んだ後に
コンフリクト解決頑張らずに PR 作り直すと楽だと思います。

@m-tmatma m-tmatma added this to the next release milestone Jun 19, 2018
@m-tmatma m-tmatma added the 🐛bug🦋 ■バグ修正(Something isn't working) label Jun 19, 2018
@kobake
Copy link
Member Author

kobake commented Jun 19, 2018

現状 #90 は x64 ブランチ向けの対応となっていて master との足並み合わせのタイミングが未定なので一旦考えずにおくことにしています。x64 ブランチ運用を続けるか否かでどちらが先にマージされるかは変わりそう。

@m-tmatma
Copy link
Member

無効になっているコードの中ですが、svnrev.h が残っています。

https://github.com/kobake/sakura/blob/38e25c2112378914b1f22da6ac444034169530ff/sakura_lang_en_US/sakura_lang_rc.rc#L8

@kobake
Copy link
Member Author

kobake commented Jun 20, 2018

あら、帰ったら消す対応しときます

@kobake kobake changed the title SVN Revision 利用箇所を削除し、代わりに GitHash を用いる [WIP] SVN Revision 利用箇所を削除し、代わりに GitHash を用いる Jun 20, 2018
@kobake
svnrev.h 参照コードを削除
34f4a47 
※ただしここのコンパイルが走ることはないことに注意（別のタイミングでこのブロックは削除することを検討）
@kobake kobake changed the title [WIP] SVN Revision 利用箇所を削除し、代わりに GitHash を用いる SVN Revision 利用箇所を削除し、代わりに GitHash を用いる Jun 20, 2018
@kobake
Copy link
Member Author

kobake commented Jun 20, 2018

svnrev.h の参照行を削除しました。
ソリューション全体で "svn", "subversion" を検索かけてみましたが、ここ以外の svn 依存箇所はなさそうでした。

ちなみにですが #define SAKURA_LANG_RESOURCE を消して #ifndef SAKURA_LANG_RESOURCE の部分がコンパイル走るようにすると sakura_rc.h が存在しないことによりコンパイルエラーになります。ここのブロックについては別件で削除するかどうかを検討すると良いと思いました。（ただし些細な問題に過ぎないので Issue は立てないでおきます。思い出したときに対応くらいで良いと思います）

@m-tmatma m-tmatma mentioned this pull request Jun 20, 2018
Closed
@m-tmatma
Copy link
Member

ちなみにですが #define SAKURA_LANG_RESOURCE を消して #ifndef SAKURA_LANG_RESOURCE の部分がコンパイル走るようにすると sakura_rc.h が存在しないことによりコンパイルエラーになります。ここのブロックについては別件で削除するかどうかを検討すると良いと思いました。（ただし些細な問題に過ぎないので Issue は立てないでおきます。思い出したときに対応くらいで良いと思います）

そういうときはチケット登録だけしておいて忘れるのがいいです。
#149 で登録しました。

@m-tmatma m-tmatma merged commit 55a7b08 into sakura-editor:master Jun 20, 2018
@kobake kobake deleted the remove-svnrev branch June 20, 2018 14:04
@kobake
Copy link
Member Author

kobake commented Jun 20, 2018

登録ありがとうございます。

僕のスタンスとしては、もっと大事なタスクがいっぱいあるはずなので、軽微なタスクで重要タスクが埋もれるのも良くないので軽微なやつは意図的にスルーする、という感じです。このへんのスタンスは人によってバラツキあって良いと思います。

@m-tmatma m-tmatma mentioned this pull request Jun 20, 2018
Open
@berryzplus
Copy link
Contributor

berryzplus commented Jun 21, 2018 •
edited
Loading

ああ、コレですね

@m-tmatma m-tmatma added the twitter twitter label Jun 21, 2018
This was referenced Jun 22, 2018
Closed
Closed
@ds14050 ds14050 added 🐛bug🦋 ■バグ修正(Something isn't working) twitter twitter labels Sep 18, 2018
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019
@m-tmatma
Merge pull request sakura-editor#147 from kobake/remove-svnrev
365b54d 
SVN Revision 利用箇所を削除し、代わりに GitHash を用いる
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-tmatma m-tmatma m-tmatma approved these changes

Assignees
No one assigned
Labels
🐛bug🦋 ■バグ修正(Something isn't working) twitter twitter
Projects
None yet
Milestone
v2.4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@kobake @m-tmatma @berryzplus @ds14050