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

Replace termion with Windows compatible crossterm #179

Merged
merged 4 commits into from
Sep 3, 2020

Conversation

remgodow
Copy link
Contributor

@remgodow remgodow commented Sep 1, 2020

Hi,
first of all thanks for creating bandwhich, I really like how easy it is to have an overview of the networking.
As much as I like Linux, I'm still a Windows guy due to gaming and work. I also wanted to try out Rust for some time, so I thought why not try adding Windows support to this awesome tool.
I have a POC working at my fork
It took some refactoring here and there, so I decided to split my work into few stages.
This is the first - using crossterm for terminal backend, as it is compatible with Windows OS.

Looking forward to make #15 into master :)

@imsnif
Copy link
Owner

imsnif commented Sep 1, 2020

Hey @remgodow, I'm very happy to see this! This looks great, and I imagine it was quite an undertaking.

This is a bit of a risky change, since it is completely untested (the TUI backend is exactly the part we're mocking in our tests - as you might have seen already). So I'd want this tested thoroughly manually.

While I'm sure you've tested this extensively, I hope you will not mind some more eyes on it. :) I'm going to run some manual tests on my machine (linux) tomorrow, and maybe workout a quick check-list of stuff to check. Then I want to find someone willing to go through that check list on a mac with this branch, to see that all is well there too. Do you happen to have access to one? If not, I'll try and find someone who does.
Then I'll go over the code itself, and also comment on the other draft PR. Other than these two, are there more things that need to be done?

I'm looking forward to making this happen and will try to devote as much of my attention as possible to it. :)

@imsnif imsnif self-assigned this Sep 1, 2020
@imsnif
Copy link
Owner

imsnif commented Sep 2, 2020

Alright. I went over all the changes in this PR and they look good. I also tested this manually on my local machine and it works great. Bundle size hasn't changed either.

I asked a friend to test this manually on a mac, I hope that happens today/tomorrow and then we're good to merge this.

@remgodow
Copy link
Contributor Author

remgodow commented Sep 2, 2020

@imsnif Nice, just in case, I should be able to ask a friend for mac tests as well, if for any reason your friend will not be able to.

As for what remains, well besides the other draft, there is an issue with building on Windows - libpnet requires WinPcap SDK, I guess we would need to automate downloading and extracting it in build.rs.

I also would like to create chocolatey package, it should not be that hard, as it will be single executable, but it would make the tool much more approachable (cargo install requires rust, visual c++, winpcap which would limit target audience to rust programmers).

@imsnif
Copy link
Owner

imsnif commented Sep 2, 2020

About winpcap - is this standard that tools automate its downloading+building? Can't we say "requires winpcap: <link to installation>"? (both in the readme and the error message).

About the chocolatey package: that would be really great.

About testing, for convenience I made a quick check list of how to manually check this for others wanting to help us. If your friend is available to check this, that would be awesome :)

  1. clone bandwhch: git clone https://github.com/imsnif/bandwhich.git
  2. checkout the PR branch:

git checkout -b remgodow-crossterm main
git pull https://github.com/remgodow/bandwhich.git crossterm

  1. cargo build

  2. sudo ./target/debug/bandwhich

  3. See that everything looks good, no text overflows and such

  4. run some traffic: wget -4 -O /dev/null http://ipv4.download.thinkbroadband.com/1GB.zip

  5. See that everything still looks good (this will simply help by filling up the tables)

  6. Press <TAB>, see that the windows switch around

  7. Press <SPACE> see that traffic is paused (enough if it says "PAUSED" in the title)

  8. press q - see that it quits

@remgodow
Copy link
Contributor Author

remgodow commented Sep 2, 2020

Can't we say "requires winpcap: "? (both in the readme and the error message).

Well, we should as for the runtime, the user needs a WinPcap/NpCap driver installed.

About winpcap - is this standard that tools automate its downloading+building?

This relates to WinPcap SDK - C Library for communicating with the driver. It is required for building libpnet on Windows.
The libpnet maintainers leave that to the windows developers as per readme

You must place Packet.lib from the WinPcap Developers pack in a directory named lib, in the root of this repository. Alternatively, you can use any of the locations listed in the %LIB%/$Env:LIB environment variables. For the 64 bit toolchain it is in WpdPack/Lib/x64/Packet.lib, for the 32 bit toolchain, it is in WpdPack/Lib/Packet.lib.

So to support CI/CD we would need to automate that, unfortunately on Windows you need to do such tasks by hand instead of good old apt-get.

@imsnif
Copy link
Owner

imsnif commented Sep 2, 2020

Aha, I see. I was hoping to do this incrementally somehow. Alright then, thanks for the explanation.

@remgodow
Copy link
Contributor Author

remgodow commented Sep 2, 2020

@imsnif Is there a way to get mac binary from travis? My friend is not familiar with Rust, so the binary would be the best option.

@imsnif
Copy link
Owner

imsnif commented Sep 2, 2020

Not trivially (that I know of). Once can get it to publish binaries to github releases (which I've been meaning to get to for a while), but I don't think for pull requests. Other than that it could be something along the lines of "get the build script to send the binary as an email somewhere", but I think it would be easier to wait for my friend. :)

@imsnif
Copy link
Owner

imsnif commented Sep 3, 2020

So, unfortunately my friend will only get to this much later today. If your friend wants to check, the list above is rather detailed and doesn't assume rust knowledge. You can add a 0 phase to it to install rust+cargo: curl https://sh.rustup.rs -sSf | sh

Otherwise, we'll wait and I'll try to find more people. Sadly around me seems to be moving to linux/windows these days :)

@remgodow
Copy link
Contributor Author

remgodow commented Sep 3, 2020

I've tested this on mac with a help of my friend, and a TeamViewer :). Looks good, tab, pause, q, ctrl-c works.

@imsnif
Copy link
Owner

imsnif commented Sep 3, 2020

Awesome. Thanks for doing this!

@imsnif imsnif merged commit 1db74c8 into imsnif:main Sep 3, 2020
@imsnif imsnif mentioned this pull request Sep 8, 2020
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