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

.vsconfigファイルを更新する #1623

Merged
3 commits merged into from Apr 17, 2021
Merged

.vsconfigファイルを更新する #1623

3 commits merged into from Apr 17, 2021

Conversation

ghost
Copy link

@ghost ghost commented Apr 12, 2021

PR の目的

.vsconfigファイルの記述を更新します。

カテゴリ

  • ドキュメント修正
  • ビルド関連
    • ローカルビルド

PR の背景

.vsconfigファイルの内容を現状に即したものに更新します。
また、更新内容と一致するようにREADME.mdとbuild.mdのビルド要件に関する記述を修正します。

PR のメリット

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

仕様・動作説明

  • 次の記述を削りました。
    • Windows 8.1 SDK と UCRT SDK
    • Windows XP サポート
    • 重複したコンポーネントIDの記述
  • build.mdの環境構築に関する内容を2017・2019双方に対応できるように変更

本PRにおけるREADME.md及びbuild.mdの記述変更は、最低限のものにとどめました。

また、今回VS2017では17763より新しいSDKがコンポーネントID一覧表に記載されていないため、インストーラからは利用できない可能性を考慮して.vsconfigを分割することとしました。
なお、VS2019で利用できる最新のSDKは19041ですが、プロジェクトの設定により18362をインストールするよう指定しています。

PR の影響範囲

ローカルビルド環境の構築

テスト内容

テスト1

手順

関連 issue, PR

#1162
#1357
#1437
#1441 (README.md及びbuild.mdの全体的な見直しはこちらで対応されるものと解しました。)
#1621 (このPRでVS2019でプロジェクトが使用するSDKバージョンの指定方法が変更されます。)

参考資料

Visual Studio のワークロードとコンポーネント ID | Microsoft Docs

berryzplus
berryzplus previously approved these changes Apr 12, 2021
Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

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

素晴らしいっす 😄

@berryzplus
Copy link
Contributor

berryzplus commented Apr 12, 2021

【 4/14追記 #1621 に付けるつもりのコメントを誤爆しました。 】

すみません、エラー(?)の対策としては #1623 のほうが良かったです。
このPRについてはお手数ですが、vsconfigの変更を戻していただけると幸いです。

「最新」の visual studio を使えるようにすることには問題もあり、若干迷いがあります。
最新の windows 機能を使えるようになるけど、vs2019でしかビルドできなくなるとか、そういうのですね。
空き容量節約したくて複数のSDKを入れたくない、は普通にあると思うので、10.0への変更自体はアリだと思っています。

現状、サクラエディタは windows vista 相当の機能にも十分には対応してないので、最新SDKを使える必要はなさそうに思います。もちろん「○○の機能を使いたいから」というのがあれば検討してバージョンを上げる方向に進む可能性もありますけど、いまのところ上げる理由がないっす。

Kohki Akikaze added 2 commits April 13, 2021 02:25
@ghost
Copy link
Author

ghost commented Apr 12, 2021

このPRについてはお手数ですが、vsconfigの変更を戻していただけると幸いです。

vsconfigの変更を不要な記述の削除のみとしました。

@ghost
Copy link
Author

ghost commented Apr 12, 2021

Kudos, SonarCloud 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

.vsconfigの変更でも実行されるんですね。

@ghost ghost marked this pull request as ready for review April 13, 2021 15:15
@k-takata
Copy link
Member

Community Editionの記述を削除していますが、どこか最低1箇所にはCommunityまたはProfessional以上ということを書いておいた方がよいのではないでしょうか。
2017にはExpress Editionもありますので。

@berryzplus
Copy link
Contributor

#1623 (comment)

なんてこった。
#1621 に付けるつもりのコメントを、誤ってここに書いてたみたいです。

@berryzplus
Copy link
Contributor

.vsconfigの変更でも実行されるんですね。

除外する必要があるか?というと微妙なので、どっちでもいいのかな、と。
変更が.vsconfigだけだったら、チェックする意味はないと思いますけどね。

@berryzplus
Copy link
Contributor

Community Editionの記述を削除していますが、どこか最低1箇所にはCommunityまたはProfessional以上ということを書いておいた方がよいのではないでしょうか。
2017にはExpress Editionもありますので。

一応 ビルドナンバー 17763 以降に相当する windows 10 SDK を備えた C++17 対応のコンパイラであれば、少なくともプログラムのビルドは可能です。

vs2017 Expressがこの条件に該当しないかというと No なはずで、Express(機能制限あり)とCommunity(=professional相当の機能を持つがprofessional付属の各種サービスは利用できない)を区別する必要はないように思います。
https://www.fenet.jp/dotnet/column/environment/1359/

詳しくはMSの営業担当にお問い合わせください、で 😃

@k-takata
Copy link
Member

.vsconfig に "microsoft.visualstudio.component.vc.atl" だか "microsoft.visualstudio.component.vc.atlmfc" が入っているので、Pro以上が必要なのかなと思いましたが、違うのかな?

@berryzplus
Copy link
Contributor

microsoft.visualstudio.component.vc.atlmfcはHTMLヘルプのビルドに必要です。

@ghost
Copy link
Author

ghost commented Apr 14, 2021

Community Editionの記述を削除していますが、どこか最低1箇所にはCommunityまたはProfessional以上ということを書いておいた方がよいのではないでしょうか。
2017にはExpress Editionもありますので。

.vsconfig に "microsoft.visualstudio.component.vc.atl" だか "microsoft.visualstudio.component.vc.atlmfc" が入っているので、Pro以上が必要なのかなと思いましたが、違うのかな?

https://docs.microsoft.com/ja-jp/visualstudio/install/workload-component-id-vs-express?view=vs-2017
リストを見る限り、ATLどころか変更前・変更後の.vsconfigに書かれた全てのコンポーネントが含まれていませんでした。
(SDKも17134までしか使えないみたいですね。)

「Professional/Community以上のエディションが必要」と加筆しておきます。

- Community/Professiona以上のエディションが必要であることを明記
- Communityエディションに限定するような記述の残存箇所を修正
@sonarcloud
Copy link

sonarcloud bot commented Apr 14, 2021

Kudos, SonarCloud 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

@k-takata
Copy link
Member

もしかしたらExpress Editionでもビルドできるのかもしれませんが、誰も試していないのならCommunity/Pro以上と書いておくのが無難でしょうね。

@berryzplus
Copy link
Contributor

そっすね・・・。しかも必要としているsdkバージョンに対応してないみたいですし、MS的にも非推奨ですし。

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

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

ドキュメント更新だけでもありだと思うのでapproveにします。
本当は、当初のvs2019版sdkバージョン変更を含めてokだと思うんですけど 😃

@ghost
Copy link
Author

ghost commented Apr 16, 2021

approveありがとうございます。
#1621 もマージへ進んだようなので、ほかに意見がなければこちらも近日中にマージしようと思います。

@ghost
Copy link
Author

ghost commented Apr 17, 2021

マージさせていただきます。
ありがとうございました。

@ghost ghost merged commit b744e3c into sakura-editor:master Apr 17, 2021
@ghost ghost deleted the feature/fix_vsconfig branch April 17, 2021 13:33
@beru beru added document ドキュメント local build labels Oct 4, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
document ドキュメント local build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants