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

support windows. #1

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

support windows. #1

wants to merge 9 commits into from

Conversation

mattn
Copy link

@mattn mattn commented Jan 11, 2013

some functions are disabled on windows.

@akiray03
Copy link
Contributor

パッチを確認したのですが、差分が大きく、Unix用コードの可読性が落ちるため、以下のいずれかの対応とさせてください。

  • Windows用にprocess_win32.cを用意し、ファイル全体を ifdef で囲む (iij/mruby-process にマージする)
  • Windows用にmruby-process-win32を作成する

mruby-processのテストは現状ありませんが、これから対応させたいと考えています
(iij/mruby にはあります: https://github.com/iij/mruby/blob/iij/test/posix/process.sh)
その際にも、Windowsの動作確認は行えないことをご了承ください。

@mattn
Copy link
Author

mattn commented Jan 16, 2013

mrbgem.rake でENVかなにかを見て process_xxx.c をビルド対象にする、みたいなのはどうでしょうか。

@akiray03
Copy link
Contributor

はい。そのような方法でも構わないと思います。

@mattn
Copy link
Author

mattn commented Jan 24, 2013

今のビルドシステムだと、srcにあるソースは全てコンパイルされてしまうので process.c から include されるファイルで拡張子が .c には出来ないです。
何か案お持ちじゃないですか。

サブフォルダなら無視してくれそうです。

@akiray03
Copy link
Contributor

  • mruby-process の直下に src と src_win32 を用意する
  • src/win32 を掘る

のどちらかでしょうか。後者で良いと思います。 (テストも同様の対処が必要かもしれないですね)

@mattn
Copy link
Author

mattn commented Jan 24, 2013

src/win32 を掘る

その場合、src/unix を作った方が #ifdef も無いし綺麗な気がしますがどうでしょうか?

@akiray03
Copy link
Contributor

お返事遅くなってすみません。

src/win32, src/unix を作る形でコードを書いてみましたが、どうしてもアドホックな対処になってしまうので、あまり良い策では無いように思えてきました。
現状のmrbgemの実装では、1つのgemの中でモードを切り替えるよりも、gemそのものを分けてしまう方が良いように思います。
VC対応の場合、 toolchain の指定と合わせて、gemの指定を mruby-process から mruby-process-win32 に書き換えることで(mrbgemの標準のスキームの中で)実現できますし。
いかがでしょうか?

@mattn
Copy link
Author

mattn commented Jan 29, 2013

私も週末そんなことを考えてました。
これらmrbgemsを分けられて困るのはmrubyビルド時くらいしかないですし、その時には意識してるはずですしね。

あとは僕がmruby-processのforkとしてmruby-process-win32を配る事に、「IIJさんとして問題無いか」なのですが、いかがでしょうか。

@akiray03
Copy link
Contributor

mruby-process は mruby と同じライセンス(MIT)としていますので、それに従って頂く限りは問題ありません。
ライセンス、著作権の表記が不充分な箇所については近日中に修正するようにします。

@mattn
Copy link
Author

mattn commented Jan 29, 2013

ありがとうございます。

@katzer
Copy link

katzer commented Apr 26, 2017

@tsahara @mattn @akiray03 We are planing add support for Windows platform and to implement Process.spawn for both Posix and Windows.

Whats the reason why this PR has not been merged and would you merge ours?

@tsahara
Copy link
Member

tsahara commented Apr 27, 2017

@katzer The reason why we have not merged this PR is that we have too many #ifdef once the patch is merged. We have discussed what is the best way to provide Process class for Windows platforms and agreed to make another mrbgem (mruby-process-win32) supports Windows platform.

@katzer
Copy link

katzer commented Apr 27, 2017

Agree. Have you considered to split the code into separate files for each platform and one for the independent things to avoid macros within method bodies?

@tsahara
Copy link
Member

tsahara commented Apr 27, 2017

akiray03 tried it and found it did not work well (he says it looks an ad hoc solution in his comment in Japanese).

take-cheeze added a commit to mrbgems/mruby-process that referenced this pull request Apr 23, 2018
* Create tmp directory for test.
* Avoid buffer overflow.
* Fix memory leaks.
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.

4 participants