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

タスク定義の記述順をビルド順に合わせたい #1776

Closed

Conversation

berryzplus
Copy link
Contributor

PR の目的

タイトル通りです。

カテゴリ

  • ビルド関連
  • その他の問題

PR の背景

#1775 とほぼ同件です。

sakura.vcxproj は元々自動生成で作られているため、記述順がビルド順と一致していません。

ビルド順はタスク同士の依存関係により決まっているので、ビルド順に合わせて記述したほうが分かりやすいように思います。

PR のメリット

PR のデメリット (トレードオフとかあれば)

仕様・動作説明

アプリの仕様・機能には影響ありません。

ビルド順序はざっくりこんな感じです。

  • ソース生成タスク
  • ビルド前処理タスク
  • ソースコンパイルタスク(ClBuild)
  • リソースコンパイルタスク(ResourceCompile)
  • マニフェストツールタスク
  • リンクタスク(Link)
  • ビルド後処理タスク

PRでは、この順番に合うように記述順序を入れ替えてようとしています。
現状で「明らかに不要」と思われるタスクがありますが、放置して順序変更だけ行います。

PR の影響範囲

MSVCビルドに影響する変更です。

テスト内容

ビルドが通れば確認は十分と思われます。

テスト1

手順

関連 issue, PR

#1730

参考資料

@sonarcloud
Copy link

sonarcloud bot commented Jan 25, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@AppVeyorBot
Copy link

@berryzplus
Copy link
Contributor Author

放置されてる理由の予想

  • そもそも見てない
  • 共感できない
    • 修正内容に共感できない
      • 実害がないなら変えなくてもいいんじゃないか?
      • 変えることによるリスクのほうが心配。
    • 修正内容の一部に共感できない
      • 不要なタスクは削ってしまえばいいんじゃないか?
      • マニフェストツールのように実行タイミングが何度かに分かれるタスクもあるので並べると分かりづらくなるんじゃないか?
  • PRの目的を理解できない

見てないんだろうなぁ・・・

@ghost
Copy link

ghost commented Jan 27, 2022

元が自動生成ファイルとなると、せっかく綺麗に並べても何かの拍子に変わったりするのではないかという心配はあります。

@sanomari
Copy link
Contributor

要らないタスクなら削ればいいのに、と思ってみてました。

それから、PreBuildって手書きタスクなので「自動生成だから」が理由だと変な感じになります。

あと vcxproj は「テンプレートから自動生成」されているだけで、元になるテンプレートは誰かが手書きしたものだったような。

@ghost
Copy link

ghost commented Jan 27, 2022

そういえばpreBuild.batとpostBuild.batはいずれも #1387#1666 で実行される処理がなくなっていましたね。

@berryzplus
Copy link
Contributor Author

どうせ削るつもりなら移動する意味がない、は了解です。
PreBuildEventの移動は、確かにいみないです。

せっかくapproveいただきましたが、こちらは閉じておきます。

@berryzplus berryzplus closed this Jan 28, 2022
@berryzplus berryzplus deleted the feature/reorder_targets branch January 28, 2022 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants