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

Fix Windows Build #195

Merged
merged 11 commits into from
Jun 21, 2023
Merged

Fix Windows Build #195

merged 11 commits into from
Jun 21, 2023

Conversation

Tps-F
Copy link
Contributor

@Tps-F Tps-F commented Jun 7, 2023

Windows上でビルドできなかったので修正
process.platformがwindowsならdelを使用するようにしました

実際に動いているところ(一応)
image

@kamataryo kamataryo self-requested a review June 8, 2023 07:52
@kamataryo
Copy link
Contributor

@Tps-F ありがとうございます、ぜひ取り込みたいです!
1点だけ、もし差し支えなければ、 以下のマトリクスを使って Windows のビルド環境も GitHub Actions に追加していただくことできますでしょうか?
https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#using-a-matrix-strategy

@kamataryo
Copy link
Contributor

@Tps-F すみません、もう一点アイデアなのですが、プラットフォームの差を吸収する方法として Node.js の fs.rmSync を使うのはいかがでしょうか?こちらですと windows/linux を問わず動作するかと思います。
https://nodejs.org/docs/latest/api/fs.html#fsrmsyncpath-options

@Tps-F
Copy link
Contributor Author

Tps-F commented Jun 9, 2023

以下のマトリクスを使って Windows のビルド環境も GitHub Actions に追加していただくことできますでしょうか?

もちろんです!学校が終わり次第行います

すみません、もう一点アイデアなのですが、プラットフォームの差を吸収する方法として Node.js の fs.rmSync を使うのはいかがでしょうか?

調べてみた限り行けそうなので、こちらも対応します!

@Tps-F
Copy link
Contributor Author

Tps-F commented Jun 9, 2023

workflowは別PRで出した方がいいでしょうか?

@kamataryo
Copy link
Contributor

@Tps-F ありがとうございます!Workflow の改修、同じプルリクエストに含めていただいて問題ないです!

@Tps-F
Copy link
Contributor Author

Tps-F commented Jun 9, 2023

@kamataryo 修正いたしました!
PRですが #203 と一部コンフリクトすると思うので分けたままにさせてもらいます

@kamataryo
Copy link
Contributor

@Tps-F ありがとうございます!
ただ、 #201 の方でテストが落ちてしまっているようです.. 😢
このプルリクエストで 「Windows ユーザーの開発者の方が参加できるようになった」状態まで改善できればと思っています。#203 は待っていただかなくて大丈夫ですので、度々申し訳ないのですが、以下の対応をお願いできますでしょうか?

  • Add windows build test #201 のコミットを cherry-pick するなどして、こちらと統合
  • npm run test:node が通るように修正

無理のない範囲で大丈夫です!

@Tps-F
Copy link
Contributor Author

Tps-F commented Jun 9, 2023

#201 の方でテストが落ちてしまっているようです..

原因はわかっていまして、今いろいろこねくり回していますので終わり次第またご連絡します!

@kamataryo
Copy link
Contributor

@Tps-F ありがとうございます!

@Tps-F Tps-F mentioned this pull request Jun 18, 2023
3 tasks
@Tps-F
Copy link
Contributor Author

Tps-F commented Jun 18, 2023

@kamataryo 統合するの忘れていたので今しました
このPRだけで対応します!

@Tps-F
Copy link
Contributor Author

Tps-F commented Jun 19, 2023

@kamataryo workflowのコミットを追加するのを忘れていました。申し訳ありません

Copy link
Contributor

@kamataryo kamataryo left a comment

Choose a reason for hiding this comment

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

@Tps-F ありがとうございます!素晴らしいです👍
今までは Mac / Linux での開発とライブラリの利用に偏ってしまっていたため、送っていただいたパッチでより多くの人に関わっていただける状態ができてとて嬉しく感じます 🙇

@kamataryo kamataryo merged commit efa47fe into geolonia:master Jun 21, 2023
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