-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Added initial cmake build support #82
base: master
Are you sure you want to change the base?
Conversation
To be continued... |
Cmake is a good build system and generally I am for it, but probably not right now. GoodbyeDPI most probably would face refactoring and Linux/macOS support in the near future, that is where cmake would be handy and I appreciate your pull request, but I would postpone to merge it. |
CMake is immediately useful, because it is not only for crossplatform build, but also for crosstoolchain one. I mean that it allows build projects with such different compilers (MSVC, GCC, Intel, clang (in MSVC and gcc modes) ) and build systems (make, nmake, ninja) with minimal compiler-specific code. Please note, that some refactoring, hardening and bug-fixing was also done. I insist on merging this as soon as possible. When the tool became cross-platform, you would be able to update the stuff to reflect the new state. |
|
||
set(GoodbyeDPI_HOST_MAXLEN 253 CACHE INTEGER "Max length of a host") | ||
set(GoodbyeDPI_MAX_FILTERS 4 CACHE INTEGER "Max length of a host") | ||
set(GoodbyeDPI_MAX_PACKET_SIZE 9016 CACHE INTEGER "Max length of a host") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not a variables and should not be configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's always a good idea to have everything hardcoded configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maximum host size is defined in RFC. Max filters should not be changed since it's internal variable for me. Packet size should not be configured in a normal circumstances either.
CMakeLists.txt
Outdated
|
||
if(MINGW) | ||
target_link_libraries(GoodbyeDPI "wsock32" "ws2_32" "ssp") | ||
set_target_properties(GoodbyeDPI PROPERTIES COMPILE_FLAGS "-Wall -Wextra -Wconversion -pie -Wformat -Werror=format-security -D_FORTIFY_SOURCE=2 -flto -mmitigate-rop -fstack-protector-strong -fno-common -fstack-check") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to have, but stack-protection won't work on all mingw builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me worked. But I used the mingw from anaconda. Recently I started using the latest MinGW, I confirm that any --stack-protector
breaks even a helloworld on the latest MinGW-w64 sjlj win32. Also I guess we should test for flags presence, cmake allows this. But IMHO a new module should be created for that, and uplifted into CMake git repo.
So feel free to remove the flag.
HOST_MAXLEN=@GoodbyeDPI_HOST_MAXLEN@u, | ||
MAX_FILTERS=@GoodbyeDPI_MAX_FILTERS@u, | ||
MAX_PACKET_SIZE=@GoodbyeDPI_MAX_PACKET_SIZE@u | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not a variables
|
||
clean: | ||
-rm -f *.o utils/*.o | ||
-rm -f $(TARGET) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not remove makefile.
@@ -23,9 +23,6 @@ WINSOCK_API_LINKAGE INT WSAAPI inet_pton(INT Family, LPCSTR pStringBuf, PVOID pA | |||
|
|||
#define die() do { sleep(20); exit(EXIT_FAILURE); } while (0) | |||
|
|||
#define MAX_FILTERS 4 | |||
#define MAX_PACKET_SIZE 9016 | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not variables.
src/goodbyedpi.c
Outdated
else { | ||
WinDivertHelperCalcChecksums(packet, packetLen, | ||
WINDIVERT_HELPER_NO_REPLACE); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split this pull request to two: one for new WinDivert support code, another for cmake.
I just realized I haven't submitted my comments. |
Added initial cmake build support.
Fixed according to the latest WinDivert. closes #79.