-
Notifications
You must be signed in to change notification settings - Fork 165
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
[WIP]build-installerとzipArtifacts.batの簡略化 #787
Conversation
iss-Win32-Release.log
Outdated
@@ -0,0 +1,415 @@ | |||
Inno Setup 5 Command-Line Compiler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不要なログが混ざっています。
sakura/postBuild.bat
Outdated
set CTAGS_ZIP=%~dp0..\installer\externals\universal-ctags\ctags-2018-09-16_e522743d-%CTAGS_PREFIX%.zip | ||
set EXE_CTAGS_NAME=ctags.exe | ||
: for x64 | ||
xcopy /Y /I %TEMP_DIR%\%BRON%\bregonig.dll %DEST_DIR%\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bregonig.dll の 64bit 版のパスが間違っていると思います。
(もともとそのドライブに移動していたら動きますが、汎用性のないコードです。 あとこれは対応が必須ではないですが、 |
@@ -0,0 +1,48 @@ | |||
# installer/externals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
一つのディレクトリに外部モジュールをまとめたいだけなら、複数のディレクトリに
配置されていた 既存の readme.md を単に名前変更して、モジュール名を表す名前に
名前変更して同じディレクトリに配置するだけでいいと思います。
一つの readme で済まそうとすると、外部モジュールが増えたときに管理しにくくなります。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
外部モジュールは増えても数個なので、管理の手間は変わらないと思ってます。セクションが別れてればマージも難しくありません。そうなると、個人的にファイル数が増えないほうが見通しがいいと考えてます。(zipをフォルダから出したのも構造をシンプルにする一環でもあります)
モジュールごとにreadme.mdを置く場合、名前をどうつけるかも問題となります。bregonig.dllのあるzipはbron412.zipだったり。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zip ファイル名.md でいいと思います。
複数ファイルに分かれていたら、追加も削除も
簡単だし、ドキュメント追加、削除漏れに簡単に気づくというメリットもあります。
以下のコードで意図してないエラーが出てます。 https://ci.appveyor.com/project/sakuraeditor/sakura/builds/22275753/job/w6r813nt34i6nter#L2321
|
CMake/MSBuildランチャー以外のすべてのバッチは、いつか不要になると思っています。 もしすぐにでもCMakeへの移行を検討できるなら、バッチ整理よりは移植に力を入れたいです。 …という未来予想図は別にして、構造整理については好意的に捉えております。やれることからやって行ったらいいのかな、と:smile: |
ご指摘を反映しました。WIPではありますが、結構複雑なバッチを整理したので漏れや不具合を指摘していただけると助かります。バッチファイルのクォートは難しい・・・
現状ログや生成物がいろいろなところに生成されて、.gitignoreできてなかったりするのが辛いので、distみたいな場所に集められないかな 🤔 |
cd /d が必要な箇所は複数あります。 |
ログファイルを誤って追加したのは、 |
ログを消してforce pushしました。interactive rebaseなれてないのでミスってたらご指摘ください。
2ファイルは修正&grepでbat内にはなさそうだと思ったんですが、他にどこかありますか? |
あれ? もともと 2ファイル修正してました? |
zipArtifacts.bat
Outdated
@@ -27,6 +27,7 @@ if "%platform%" == "x64" ( | |||
set ALPHA=0 | |||
) | |||
|
|||
call "tools\find-tools.bat" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここは %~dp0 をつけたほうがいいと思います。
zipArtifacts.bat
Outdated
) | ||
if exist "%WORKDIR_ASM%" ( | ||
rmdir /s /q "%WORKDIR_ASM%" | ||
"%CMD_7Z%" "%~dp0%OUTFILE_EXE%" .\installer\warning-alpha.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"%~dp0%OUTFILE_EXE%" と 165行目では、OUTFILE_EXE に対する扱い方が異なるので
例えば
set FULPATH_OUTFILE_EXE=%~dp0%OUTFILE_EXE%
みたいな定義を 150行目ぐらいで定義したうえで、165行目および 179行目でも使うようにしたほうがいいです。
同じパスを操作しているのに cd /d する前と後で扱い方が変わるとわかりにくいです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OUTPUTFILE_EXEを絶対パスにしました
if exist "%ISS_LOG_FILE%" ( | ||
copy /Y /B "%ISS_LOG_FILE%" %WORKDIR_LOG%\ | ||
) | ||
setlocal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この setlocal は何のために入っていますか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
カレントディレクトリをendlocalで復元するためです。(7zに与えるファイルリストの中は環境変数が使えないようなので、cdで移動して環境差を吸収しています。)
delの形式を短い方に修正
バッチ整理の一環としてzipArtifacts.batとartifactsList_exe.txt、artifactsList_log.txtをどこかのディレクトリに動かしたいのですが、ふさわしい場所(or新たなディレクトリ名)はなにかあるでしょうか? sakuraディレクトリには入れないほうがいいのかな・・・ |
前提的な部分で疑問点がいくつかあるので確認したいです。
解凍ユーティリティが7-zipになったのはぼくが推したせいもあると思ってます。 あと、外部成果物はexternals配下にフォルダ切っていれるのがいいと思います。 |
個人的には sakura で問題ないと思います:smile: |
本文で「問題点」とされてる事項についての見解。
「zipはCIで正しくできれば問題ではない」に同意です。
めんどくさいなら手を出さない、というのも一つの方策です。
ハッシュ生成は生成物をインターネット配信するうえでのマナーだと思います。 |
|
ぎゃー。このPR、postBuild.bat触るんですね。
タイミングが合わなくて・・・的な事情ですね。
作らないようにしたら appveyor の時間を10秒くらい削れるんじゃないと思ってますw
7z.exe(455KB) は OLE経由 で 7z.dll(1,639KB) に依存してそうです。
想定しているよりもソースツリーの容量が大きくなりそうで驚いています。 readmeに関して、熟知しているツールのドキュメントを見る機会はほぼないのかな?と思っています。リリース時のメンテナンスは、grepを活用すればバージョン確認程度ならファイルを開かなくても一括でできるし、パスにツール名が入る分フォルダが分かれていたほうが便利かな?と思ってます。 そのへんはリリース工程回してみてぼちぼち判断でいいと思います。
言ってることが分かってきました。
|
そうですね><、なかなか忙しくて他のPRに目を通せていないので他にも被っていたらごめんなさいです。
7za.exeがスタンドアローン版としてDLLなしで使えるみたいです。こちらは740KB程度で、機能面を考えるとこれぐらい入れちゃっていいだろうと思ってます。 nugetはVisual Studioが入っている環境なら全て存在していそうな気がします。
現状でもpullやcheckoutの時間はそれほどかかっておらず、数MB程度ならshallow cloneするようにすれば気にするほどでもないと思ってます。 |
ds14050さんにapproveを貰ったのでマージしてしまおうと思います。
いいような気がします。
ないんですよね(笑 ただ、 vs2017 には nugetのすべての機能が組み込まれているので、
気にしてるのは直に まぁ、細かいところは気付いた人がメンテする、として進めちゃっていいのかな?とも思っています。 |
このPRは賞味期限切れだと思うので閉じておきます。 |
#681 を置き換えるPRです。
問題点