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

Move carton command implementation into CartonDriver module #459

Merged
merged 2 commits into from
May 20, 2024

Conversation

omochi
Copy link
Contributor

@omochi omochi commented May 20, 2024

課題

carton コマンドは main.swift に実装が書かれています。
しかし、executable targetに実装を書くと、それがテストしづらくなります。
テストターゲットが executable target に依存すると、
しばしばリンクの問題が生じるからです。

最近はSwiftPMが頑張っているのでビルドできたりしますが、
Wasmターゲットなど新たな環境では壊れたりします。
それも最近直されたりはしましたが、将来のリスクはあるでしょう。

提案

CartonModule モジュールを導入し、cartonコマンドの実装を全て移します。
carton executable targetはそれをimportしてエントリポイントを呼び出すだけにします。

@omochi omochi marked this pull request as ready for review May 20, 2024 00:48
Package.swift Outdated
@@ -13,6 +13,7 @@ let package = Package(
products: [
.library(name: "SwiftToolchain", targets: ["SwiftToolchain"]),
.library(name: "CartonHelpers", targets: ["CartonHelpers"]),
.library(name: "CartonModule", targets: ["CartonModule"]),
Copy link
Member

@kateinoigakukun kateinoigakukun May 20, 2024

Choose a reason for hiding this comment

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

Suggested change
.library(name: "CartonModule", targets: ["CartonModule"]),
.library(name: "CartonDriver", targets: ["CartonDriver"]),

~Module is too general. Given that the tool invokes several underlying commands, Driver sounds reasonable to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

説明を忘れていました。

carton コマンドの実装なので Carton モジュールにしたかったのですが、
macOSではファイルシステムがcase insensitiveなため、
Sources/Carton を作れませんでした。

そこで、 swift-collectionsなどで用いられる、
ただモジュール名を型名をずらす時のsuffixとして Module をつける方法を真似しました。

https://github.com/apple/swift-collections/blob/03eb23630b85dfd553a403a8dc3ae0c7b86f182f/Package.swift#L223


とはいえ、これを driver と呼ぶことにするのは良いと思います。
Swift compiler の driver と frontend のコンセプトと似ていますし。

そのように変更します。

@kateinoigakukun kateinoigakukun changed the title CartonModuleを導入し、cartonコマンドの実装を移す Move carton command implementation into CartonDriver module May 20, 2024
@kateinoigakukun kateinoigakukun merged commit bb9a91c into swiftwasm:main May 20, 2024
4 checks passed
@omochi omochi deleted the carton-module branch May 20, 2024 06:40
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.

2 participants