这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@hoijui
Copy link

@hoijui hoijui commented May 15, 2019

This includes:

  • fix non-linux builds
  • fix Travis-CI build
  • add Travis-CI build on OSX
  • add Travis-CI cross-build for Windows

At the current state, the library faisl to build on windows, which causes any dependent packages that build on windows to fail too.

@hoijui
Copy link
Author

hoijui commented May 15, 2019

This change made the library fail to compile under windows, because syscall is OS dependent:
8af0ab9#diff-d791be678c8d8fb73ef58a9a7bf54b45R34

I chose to use a library instead, which removes the burden from us to implement code correctly for all possible platforms.

@asahasrabuddhe
Copy link
Collaborator

@hoijui

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.

@asahasrabuddhe
Copy link
Collaborator

@hoijui I've integrated your changes in my branch. Thanks!

@hoijui
Copy link
Author

hoijui commented May 16, 2019

hey! :-)
he recently included your pull request, so I though he might use mine too.
I saw that you are wokring on it more these days, but of course people are using his github URL as dependency. It also appears somewhere in the dependency tre of a project I am wokring on. I don't know how it gets in there (is there a way to print the dep.-tree?) but I woudl rather not go aroudn all projects trying to get them to change to your URL ... which is why I would like to have at least fixes published to here.
optimally, he might want to give you push rights... (wink, wink @gosuri ;-) ).

Where I come from, 200KB is not a big dependency. And more modular code is better.

I do not think using the library solves the issue as the library uses the same code i.e. the system calls.

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,
so if there are any more problems, it can be solved quicker and more easily (in case it would be someone else then us, fixing it)

@gosuri
Copy link
Owner

gosuri commented May 16, 2019

Hey @asahasrabuddhe, i merged your PR and looks like @hoijui's patch made it too

@asahasrabuddhe
Copy link
Collaborator

Hey @gosuri, could you add me as a maintainer here and on the progress repo? Would help speed up things.

@asahasrabuddhe
Copy link
Collaborator

@hoijui

Where I come from, 200KB is not a big dependency. And more modular code is better.

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.

I do not think using the library solves the issue as the library uses the same code i.e. the system calls.

That is definitely wrong, as I proved with the successfull windows cross-compilation. The library uses different code for windows and linux.

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.

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,

This seems like a good idea. Would do this soon.

so if there are any more problems, it can be solved quicker and more easily (in case it would be someone else then us, fixing it)

Thanks a lot for the PR and making this library even better!

@hoijui
Copy link
Author

hoijui commented May 17, 2019

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:
https://softwareengineering.stackexchange.com/a/37010/30421
I would not do it the way you did, but eihter use termbox or write a new library that does less (though using termbox seems like a better idea in this case).

better_code_structure > (less_dependencies && shorter_build && smaller_binary)

but I am fine with this thing working again now, and if you keep maintainig it... it's your shot of course.
good luck!


Hey @asahasrabuddhe, i merged your PR and looks like @hoijui's patch made it too

it made it, but half of it got reverted (the part of using termbox)

@asahasrabuddhe
Copy link
Collaborator

@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 :)

@hoijui
Copy link
Author

hoijui commented May 18, 2019

ah... he mentioned placing it under the vendor directory. this basically means, you put the whole library source code under that directory, and then you delete eveyrthing you don't need, and you document from which commit of the original repo you copied the sources. that woudl eb a much better solution indeed.

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.

@hoijui
Copy link
Author

hoijui commented May 18, 2019

... like me being agressive here. ;-)

@asahasrabuddhe
Copy link
Collaborator

@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.

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.

3 participants