-
Notifications
You must be signed in to change notification settings - Fork 88
Fix windows build #26
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
Conversation
... because it is platform agnostic -> works on windows too
|
This change made the library fail to compile under windows, because syscall is OS dependent: I chose to use a library instead, which removes the burden from us to implement code correctly for all possible platforms. |
|
I do not think using the library solves the issue as the library uses the same code i.e. the system calls. However, I like the idea where you have added the travis builds. I have already fixed most of the issues on my fork in #22 I'd really appreciate if you could do a PR there. I don't think @gosuri is interested in maintaining this package as my request to him earlier has gone unanswered. Let's collaborate in one place and make this library better. Hopefully, when Greg is free, we can push the changes back to the upstream. |
|
@hoijui I've integrated your changes in my branch. Thanks! |
|
hey! :-) Where I come from, 200KB is not a big dependency. And more modular code is better.
That is definitely wrong, as I proved with the successfull windows cross-compilation. The library uses different code for windows and linux. I assume you have a better feeling what is right in the Goworld, as I am new to it, but at the very least, I would recommend to include links to the code on https://github.com/nsf/termbox-go in comments, |
|
Hey @asahasrabuddhe, i merged your PR and looks like @hoijui's patch made it too |
|
Hey @gosuri, could you add me as a maintainer here and on the progress repo? Would help speed up things. |
Agreed but we are only ever going to use just the single function and including an entire library is kind of heavy considering that it would add up to the size of the final binary too.
Even my code cross compiles successfully on travis. Take a look. I have used the same config as you. Also, my implementation uses different code on windows and linux. Notice the build +windows and build !windows comments on top of writer.go and writer_windows.go This ensures that the specific files are used on the specific architectures.
This seems like a good idea. Would do this soon.
Thanks a lot for the PR and making this library even better! |
|
I never doubted that your code works fine; you doubted that mine works, even though it was proven that it does. ;-) about copy&paste, I agree with this: but I am fine with this thing working again now, and if you keep maintainig it... it's your shot of course.
it made it, but half of it got reverted (the part of using termbox) |
|
@hoijui it seems you misunderstood me. I never doubted your code. Using termbox was going to fix this 100% but what I meant instead was that it wasn't the solution for the current scenario. Also, the decision to not use Termbox isn't mine. That was something that the repo owner had made way back. Refer #4 (comment) Looking forward to collaborate with you once again :) |
|
ah... he mentioned placing it under the I know this is a cultural thing, but I personally find it very limitting for personal growth and inter-human communication reasons, to not aknowledge ones own mistakes; it prevents learning from them. Therefore, I won't participate in that. Making mistakes is part of being human, it does not make one stupid or not respect-worthy or anything like that. Collaborating brings mistakes to light, which is the beauty of it. |
|
... like me being agressive here. ;-) |
|
@hoijui It's good that I don't care. I don't necessarily think I am making a mistake here. Also, I am done arguing with you. |
This includes:
At the current state, the library faisl to build on windows, which causes any dependent packages that build on windows to fail too.