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

APPVEYOR_REPO_COMMIT の利用をやめる (APPVEYOR_SHORTHASH も廃止する) #886

Closed
wants to merge 17 commits into from
Closed

APPVEYOR_REPO_COMMIT の利用をやめる (APPVEYOR_SHORTHASH も廃止する) #886

wants to merge 17 commits into from

Conversation

m-tmatma
Copy link
Member

@m-tmatma m-tmatma commented Apr 30, 2019 •
edited
Loading

APPVEYOR_REPO_COMMIT の利用をやめる (APPVEYOR_SHORTHASH も廃止する)

背景

#821 で Azure Pipelines で環境変数の定義を検討中だが、
Azure Pipelinse の Predefined variables では
$(Build.SourceVersion) (YMLの場合) あるいは BUILD_SOURCEVERSION (バッチファイルの場合)
という appveyor と別の環境変数が割り当てられている。

しかも

git show -s --format=%H という git コマンドで取得できるので二重管理になっているので
APPVEYOR_REPO_COMMITを利用するのをやめる。
同様に APPVEYOR_SHORTHASH も 対応する GIT_SHORT_COMMIT_HASHという変数があるので廃止する

2019/06/15 追記

TEMP_GIT_SHORT_COMMIT_HASH と TEMP_GIT_COMMIT_HASH を一時的に導入して
動作を維持するようにしました。

別の PR で GIT_SHORT_COMMIT_HASH と GIT_COMMIT_HASH に名前変更して仕様変更します。

仕様が変更になる点

ローカルビルドの場合は APPVEYOR_REPO_COMMIT が空だったので zip ファイル名には
commit hash が含まれなかったが、この PR 後は GIT_SHORT_COMMIT_HASH を
参照するようになるので Git が有効な場合には zip ファイル名に short hash が含まれるようになる。

@m-tmatma m-tmatma added the specification change ■仕様変更 label Apr 30, 2019
@m-tmatma m-tmatma added this to the v2.4.0 milestone Apr 30, 2019
@m-tmatma
Copy link
Member Author

この頃は azure pipelines 通ってた。
https://dev.azure.com/sakuraeditor/sakura/_build/results?buildId=323

@berryzplus
Copy link
Contributor

この頃は azure pipelines 通ってた。
https://dev.azure.com/sakuraeditor/sakura/_build/results?buildId=323

Pull-Requestビルドでも通ってるものがありますね。
https://dev.azure.com/sakuraeditor/sakura/_build/results?buildId=368

履歴を眺めるとPull-Request起因のビルドが軒並みコケてるように見えるので、conditionの条件指定になんらかの想定外要因があるのを疑っています。全部ダメなわけじゃないから違いはなんだ！なんですけどｗ

ちなみにこのPRについては、変更趣旨了解で問題なさそうと判断しています。マクロ定義の入替なので最低限grep確認とかする必要があると思ってて、作業できてないからレビューコメントしてない感じです。

@m-tmatma
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@m-tmatma
Copy link
Member Author

https://docs.microsoft.com/en-us/azure/devops/pipelines/repos/github?view=azure-devops&tabs=yaml#comment-triggers で /azp という省略形が使える。

@m-tmatma m-tmatma mentioned this pull request May 18, 2019
Closed
@m-tmatma
Copy link
Member Author

またビルドに失敗している。

@m-tmatma
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@berryzplus
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@m-tmatma m-tmatma force-pushed the feature/suspension-use-APPVEYOR_REPO_COMMIT branch 2 times, most recently from 486187c to 40027f5 Compare May 22, 2019 13:07
@m-tmatma m-tmatma force-pushed the feature/suspension-use-APPVEYOR_REPO_COMMIT branch from 40027f5 to 872801b Compare May 26, 2019 06:53
@m-tmatma
Copy link
Member Author

rebase しました。レビューお願いします。

@m-tmatma m-tmatma added appveyor azure pipelines CI appveyor など CI 関連 【ChangeLog除外】 labels May 26, 2019
@berryzplus
Copy link
Contributor

2256b58 commit に対する有効化条件の誤りを修正

このコミットの意図がよく分らんです。
(コミットメッセージにある「誤り」というのもよく分らんです。)

CAboutDlg に表示する情報の仕様変更にあたると思うんですけど、
なんで変えないといけないんでしたっけ？

たぶん、何か問題がある⇒変えよう、なはずで、
何を問題と考えたか、どう対策しようとしてるのかが共有できてないと思います。

@m-tmatma
Copy link
Member Author

ローカルビルドでは、ハッシュが定義されますが
GitHub の URL は定義されません。

これまでのように、ハッシュだけで判断すると GitHub のURL のみ
定義されていないので、URL は表示されず、説明のテキストだけ
表示されて微妙な表示になります。

@berryzplus
Copy link
Contributor

なんか問題があるように思えてきました。

ローカルビルドでは、ハッシュが定義されますが
GitHub の URL は定義されません。

これまでのように、ハッシュだけで判断すると GitHub のURL のみ
定義されていないので、URL は表示されず、説明のテキストだけ
表示されて微妙な表示になります。

この説明だけ読むと、これまで通りハッシュだけで判断できるように、GitHub の URLがないときはハッシュを定義しない をバッチ側に組み込めばよい気がします。

つまり、「sakura_rc.rcへの変更は本質的には必要ない」と読めます。。。

@m-tmatma
Copy link
Member Author

m-tmatma commented May 30, 2019 •
edited
Loading

表書いてみたけどあまりわかりやすくない。
でもせっかく書いたので残しておきます。

この PR 前

ローカルビルド (git なし) ローカルビルド (git あり) Appveyor ビルド
GIT_COMMIT_HASH × 〇 〇
GIT_SHORT_COMMIT_HASH
(GIT_COMMIT_HASH に依存)		 × 〇 〇
APPVEYOR_REPO_COMMIT × (この変数を使用) × (この変数を使用) 〇
APPVEYOR_SHORTHASH
(APPVEYOR_REPO_COMMIT に依存)		 × × 〇
APPVEYOR_PULL_REQUEST_HEAD_COMMIT × × 〇
GITHUB_COMMIT_URL
(APPVEYOR_REPO_COMMIT に依存)		 × × 〇
GITHUB_COMMIT_URL_PR_HEAD × × 〇 (APPVEYOR_PULL_REQUEST_HEAD_COMMIT で定義)

この PR 後

ローカルビルド (git なし) ローカルビルド (git あり) Appveyor ビルド
GIT_COMMIT_HASH × (この変数を使用) 〇 (この変数を使用) 〇
GIT_SHORT_COMMIT_HASH
(GIT_COMMIT_HASH に依存)		 × 〇 〇
APPVEYOR_REPO_COMMIT
(appveyor 定義変数)		 × × ×
APPVEYOR_SHORTHASH
(APPVEYOR_REPO_COMMITに依存)		 × × ×
APPVEYOR_PULL_REQUEST_HEAD_COMMIT × × 〇
GITHUB_COMMIT_URL
(GIT_COMMIT_HASH に依存)		 × × 〇
GITHUB_COMMIT_URL_PR_HEAD × × 〇 (APPVEYOR_PULL_REQUEST_HEAD_COMMIT で定義)

@m-tmatma
Copy link
Member Author

m-tmatma commented May 30, 2019 •
edited
Loading

この PR の目的は、
appveyor 固有変数である APPVEYOR_REPO_COMMIT を使っていたのをやめて
sakura editor で定義した GIT_COMMIT_HASH を使うことです。

APPVEYOR_REPO_COMMIT は appveyor 環境でしか定義されないものです。
でも APPVEYOR_REPO_COMMIT は GIT_COMMIT_HASH と等しい値です。
なので利用するのをやめます。

ローカルビルドのとき、あるいは Azure Pipelines ビルドのときは GIT_COMMIT_HASH が
利用できますが、APPVEYOR_REPO_COMMIT は appveyor でしか利用できません。

APPVEYOR_PULL_REQUEST_HEAD_COMMIT に関しては、Git から情報が取れないので (取り方を知らないので) そのまま使います。

一方 、 GITHUB_COMMIT_URL というのは appveyor の変数 (APPVEYOR_REPO_NAME) に依存しているので現状 appveyor ビルドのときのみ有効になる変数です。定義値が appveyor に依存はしていますが、
Azure Pipelines にある同様の変数から定義内容を変えることは可能です。

GITHUB_COMMIT_URL の変数が使えないからと言って、別に GIT_COMMIT_HASH (ソースツリーのコミットハッシュ) の値が無効になるわけではありません。
ローカルで Git 環境でビルドするときには利用できるからです。

それではなぜ APPVEYOR_SHORTHASH の部分を変えるかというと、この PR のもともとの動機である Azure Pipelines に対応準備のため、appveyor の変数に直接依存する部分をなくして、sakura editor で定義した変数に依存するようにして、appveyor の変数に対する参照を隠ぺいして局所化するためです。

@berryzplus
Copy link
Contributor

現状のバージョン表示の仕様に、潜在的な「?」要素が潜んでいたことは分かりました。

とりあえず一旦これで行くか、環境変数の仕様(=何を意味する変数か？)を整理して各変数を個別に再定義して行くか、どちらが良いかを迷っています。

個人的には後者がいいと思っていますが、結構めんどくさそうなのがネック。

バージョン情報として表示する情報である以上、「各変数が何を意味するものか分からん」ということはないと思っています。何を意味する情報かがわかるなら、元ネタとして最低限必要な情報を特定できます。派生情報に当たる部分は環境に依存しない方法で共通化出来ます。

さて、どうしたものか。

@m-tmatma
Copy link
Member Author

全体の整理は #821 で行います。
その前に要らないものが、あれば削除していきます

@m-tmatma
Copy link
Member Author

一方 、 GITHUB_COMMIT_URL というのは appveyor の変数 (APPVEYOR_REPO_NAME) に依存しているので現状 appveyor ビルドのときのみ有効になる変数です。

git config --get remote.origin.url
を使えば remote URL をとれるので、頑張ればローカルビルドでも取れると思います。
ただ git プロトコルの場合の考慮とか、末尾の .git をとる対応とかしないといけないので
めんどくさいです。

@berryzplus
Copy link
Contributor

ただ git プロトコルの場合の考慮とか、末尾の .git をとる対応とかしないといけないので
めんどくさいです。

gitプロトコルを考慮すべきかどうかは要件次第だと思います。
リンク先は Internet Explorer で開ける Webサイト でなければならない
という要件を決めたのは誰でしょう？
「そんな要件はない！」がぼくの見解です 😄

@m-tmatma
Copy link
Member Author

m-tmatma commented Jun 1, 2019

GITHUB_COMMIT_URL の名前からして、GitHub の対応するコミットに対応する URL という前提です。
remote URL ではないです。remote URL という項目だったら git プロトコルでも http でもありなんですが。

@m-tmatma m-tmatma force-pushed the feature/suspension-use-APPVEYOR_REPO_COMMIT branch from 3cbcfbb to be371a7 Compare July 21, 2019 01:36
@m-tmatma
Copy link
Member Author

リンクとしては作動しないけどテキストとしては表示される

だいぶん前のことなのでちょっと問題の内容を書き間違えた。

これ自体に問題はないように思います。
このコントロールは本来 LTEXT(＝ただのラベル) だからです。
ラベルだから、仮にリンクさせる対象がなくても表示していいと思います。

https://github.com/sakura-editor/sakura/pull/886/files#diff-67955f03bb0bbb169f93ff6fa4998095

もともとは APPVEYOR_SHORTHASH を参照してラベルを表示するかしないかの条件コンパイルです。
でもこの PR では APPVEYOR_SHORTHASH を廃止して、GIT_SHORT_COMMIT_HASH に置き換えます。

そうすると、appveyor では APPVEYOR_SHORTHASH は定義されていますが、
Azure Pipelines では APPVEYOR_SHORTHASH は定義されていません。

この PR で APPVEYOR_SHORTHASH → GIT_SHORT_COMMIT_HASH に変えた場合
appveyor では動作が変わらないですが、

Azure Pipelines では GIT_SHORT_COMMIT_HASHが定義されているので、
IDC_STATIC_URL_GITHUB_CAPTION が定義されますが、
IDC_STATIC_URL_GITHUB_COMMIT が定義されないということになります。

単純に APPVEYOR_SHORTHASH → GIT_SHORT_COMMIT_HASH にすると
azure pipelines では IDC_STATIC_URL_GITHUB_CAPTION は表示されるが
IDC_STATIC_URL_GITHUB_COMMIT は表示されないという
これまでの動きと異なることになります。

m-tmatma added 3 commits July 21, 2019 11:01
@m-tmatma
TEMP_GIT_SHORT_COMMIT_HASH, TEMP_GIT_COMMIT_HASH を導入
78d886f
@m-tmatma
いったん master の状態に戻す
95f541c 
git checkout master -- sakura_core/sakura_rc.rc
git checkout master -- sakura_lang_en_US/sakura_lang_rc.rc
@m-tmatma
TEMP_GIT_SHORT_COMMIT_HASH に差し替え
4a0c89a
@AppVeyorBot
Copy link

✅ Build sakura 1.0.2063 completed (commit cbf1348762 by @m-tmatma)

@m-tmatma
Copy link
Member Author

ぐちゃぐちゃになったので、動作確認後、PR 作り直します。

方針は

  1. TEMP_GIT_SHORT_COMMIT_HASH と TEMP_GIT_COMMIT_HASH を一時的に導入する
  2. TEMP_GIT_SHORT_COMMIT_HASH と TEMP_GIT_COMMIT_HASH は githash.bat で定義する
  3. APPVEYOR_SHORTHASH → TEMP_GIT_SHORT_COMMIT_HASH への一括置換
  4. APPVEYOR_REPO_COMMIT → TEMP_GIT_COMMIT_HASH への一括置換
  5. 環境変数 APPVEYOR が定義されているときに TEMP_GIT_SHORT_COMMIT_HASH は GIT_SHORT_COMMIT_HASH (GIT_COMMIT_HASH) をもとに定義する
  6. 環境変数 APPVEYOR が定義されているときに TEMP_GIT_COMMIT_HASH は GIT_COMMIT_HASH をもとに定義する

@AppVeyorBot
Copy link

✅ Build sakura 1.0.2064 completed (commit 3e6a569248 by @m-tmatma)

@AppVeyorBot
Copy link

✅ Build sakura 1.0.2066 completed (commit 26d8239661 by @m-tmatma)

@m-tmatma m-tmatma mentioned this pull request Jul 21, 2019
Merged
@m-tmatma
Copy link
Member Author

#969 を作成したので、クローズ

@m-tmatma m-tmatma closed this Jul 21, 2019
@berryzplus
Copy link
Contributor

なんかすんません。

このあたりのレスをみて「なんで？」と思いました。

この PR で APPVEYOR_SHORTHASH → GIT_SHORT_COMMIT_HASH に変えた場合
appveyor では動作が変わらないですが、
Azure Pipelines では GIT_SHORT_COMMIT_HASHが定義されているので、
(中略) が定義されないということになります。

「appveyor では動作が変わらないですがazure pipelinesでは動作が変わる」と言ってるように見えました。それってつまり、本来の目的を達成しとらんような気がするんです。（まぁ、そういうわけじゃないんだと思いますが。

目的：
APPVEYOR_SHORTHASH (appveyor専用、azure pipelinesには無い)
　↓
GIT_SHORT_COMMIT_HASH (azure pipelinesにもある同等の値を使う)

@m-tmatma
Copy link
Member Author

azure pipelines や ローカルビルドの振る舞いを変えないための経過措置です
リファクタリングだけに絞ったからです。

別の PR で azure pipelines 対応します

@m-tmatma
Copy link
Member Author

↑ #969 の話

@m-tmatma m-tmatma mentioned this pull request Feb 11, 2020
Merged
@m-tmatma m-tmatma deleted the feature/suspension-use-APPVEYOR_REPO_COMMIT branch February 11, 2020 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
appveyor azure pipelines CI appveyor など CI 関連 【ChangeLog除外】
Projects
None yet
Milestone
v2.4.0
Development

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