gamedig / rust-gamedig

Game Server Query Library.
https://crates.io/crates/gamedig
MIT License
35 stars 10 forks source link

Add packet capture functionality and many more CLI improvements #182

Closed Douile closed 5 months ago

Douile commented 5 months ago

Continuation of #162.

Adds packet capture functionality to the CLI without the need for external tools (like wireshark).

cainthebest commented 5 months ago

This PR is looking really good, I think setting some goals for this will mark the process toward this being ready.

Douile commented 5 months ago

Remove debug symbols to streamline the binary.

I'm not sure that we should do this, debugging symbols are very useful if something goes wrong. Although maybe we could use split-debugging symbols. (stripping got the binary down to 2.5M from 5.3M on my release binary).

Look into specific compiler flags suitable for different architectures/OS.

I'm not sure this is necessary for this PR but more of a stretch goal.

Achieve a high level of accuracy in emulating missing packets for pcap.

Could you elaborate more on what we're currently missing (sorry I wrote this code a while ago).

Everything else LGTM! :rocket:

cainthebest commented 5 months ago

I'm not sure that we should do this, debugging symbols are very useful if something goes wrong. Although maybe we could use split-debugging symbols. (stripping got the binary down to 2.5M from 5.3M on my release binary).

Im not that familiar on this usage in release but I think it worth looking into as its close to 50% of the size, we can always compile the build with them.

I'm not sure this is necessary for this PR but more of a stretch goal.

yeah that true, will remove from the list

Could you elaborate more on what we're currently missing (sorry I wrote this code a while ago).

Well you could say nothing is missing but currently the emulation isn't quite right. I don't think its a big issue as we can see that they are generated.

Was mentioned here https://github.com/gamedig/rust-gamedig/pull/162#issuecomment-1893615956

Edit: I think we can mark this as a known issue on merge

cainthebest commented 5 months ago

I'm holding of pushing anything ATM because I don't want to conflict with any local changes you might have.

Feel free to push, I have been testing the workflow from a different repo

cainthebest commented 5 months ago

I think this is now at the edge of the scope for this PR, we have a few things we can improve at a later date but we have made the goal of the functionality we set out. As this is quite a big change I have requested a review, pls feel free to nitpick

cainthebest commented 5 months ago

Had another look over and LGTM, are you happy to merge this @CosminPerRam ?

CosminPerRam commented 5 months ago

Hell yeah! I'll let you guys do the honor (: