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

Full support for Win32 and many other enhancements #11

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

Conversation

katzer
Copy link

@katzer katzer commented Jun 13, 2017

@tsahara @mattn @akiray03

As hinted in #1 here comes our PR

The PR adds full support for Win32 platform and various enhancements for posix platform. Honestly its more a rewrite than just a PR.

We would be happy to get some feedback and hope you will merge it. See the README for whats supported.

https://github.com/appPlant/mruby-process/tree/windows

@katzer katzer force-pushed the windows branch 2 times, most recently from 022992f to 5cea161 Compare June 14, 2017 22:04
@katzer katzer force-pushed the windows branch 2 times, most recently from 3a06f78 to 8cb1847 Compare June 15, 2017 09:08
@katzer katzer changed the title Full support for Win32 and many other enhancements Full support for Win32 and many other enhancements (WIP) Jun 15, 2017
MrBlaTi and others added 3 commits June 15, 2017 17:38
fixed wrong argument resolving for cases where more than one argument is provided, but dln cant find anything
@katzer katzer changed the title Full support for Win32 and many other enhancements (WIP) Full support for Win32 and many other enhancements Jun 18, 2017
@katzer
Copy link
Author

katzer commented Jun 28, 2017

@tsahara, @iij Any comments?

@tsahara
Copy link
Member

tsahara commented Jun 29, 2017

Full support for Win32 platform is great. Nice work. But this PR has a few issues:

  • You must not change license terms when you copy a large part of a file from other projects.
  • It breaks *BSD support.
  • The patch is too big to review at once. If it adds WIN32 support only, I can merge it without detailed review because it won't break anything works well currently.

@katzer
Copy link
Author

katzer commented Jun 29, 2017

@tsahara Thanks for your comments.

You must not change license terms when you copy a large part of a file from other projects.
It breaks *BSD support.

Both repos are under MIT license. What exactly do you mean and what needs to me modified?

The patch is too big to review at once. If it adds WIN32 support only, I can merge it without detailed review because it won't break anything works well currently.

OK, but how we can go from here? A merge without detailed review doesn't seem possible. On the the other side I want to avoid having a second mruby-process mgem. We've added lots of tests and ci integration to make sure that everything works as expected.

In case of questions, suggestions and future support we're open to incorporate. We had to touch all the files so a PR which adds just a file or a method isn't possible.

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