-
Notifications
You must be signed in to change notification settings - Fork 687
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
Some addition for cmake #596
base: master
Are you sure you want to change the base?
Conversation
Add wpcap files to soem include list
ping @hefloryd |
CMakeLists.txt
Outdated
@@ -25,12 +25,14 @@ if(WIN32) | |||
include_directories(oshw/win32/wpcap/Include) | |||
if(CMAKE_SIZEOF_VOID_P EQUAL 8) | |||
link_directories(${CMAKE_CURRENT_LIST_DIR}/oshw/win32/wpcap/Lib/x64) | |||
set(OS_LIBS ${CMAKE_CURRENT_LIST_DIR}/oshw/win32/wpcap/Lib/x64/wpcap.lib ${CMAKE_CURRENT_LIST_DIR}/oshw/win32/wpcap/Lib/x64/Packet.lib Ws2_32.lib Winmm.lib) |
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.
Please fix indentation
CMakeLists.txt
Outdated
elseif(CMAKE_SIZEOF_VOID_P EQUAL 4) | ||
link_directories(${CMAKE_CURRENT_LIST_DIR}/oshw/win32/wpcap/Lib) | ||
set(OS_LIBS ${CMAKE_CURRENT_LIST_DIR}/oshw/win32/wpcap/Lib/wpcap.lib ${CMAKE_CURRENT_LIST_DIR}/oshw/win32/wpcap/Lib/Packet.lib Ws2_32.lib Winmm.lib) |
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.
Please fix indentation
CMakeLists.txt
Outdated
endif() | ||
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /D _CRT_SECURE_NO_WARNINGS") | ||
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /WX") | ||
set(OS_LIBS wpcap.lib Packet.lib Ws2_32.lib Winmm.lib) | ||
#set(OS_LIBS wpcap.lib Packet.lib Ws2_32.lib Winmm.lib) |
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.
Please remove rather than comment obsolete code
@@ -76,6 +78,8 @@ add_library(soem STATIC | |||
${OSHW_EXTRA_SOURCES}) | |||
target_link_libraries(soem ${OS_LIBS}) | |||
|
|||
target_compile_definitions(soem PUBLIC __STDC_LIMIT_MACROS __STDC_CONSTANT_MACROS) |
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 defines should only be set if the compiler is a C++-compiler, and preferably only if a C++-standard older than C++11 is used since it seems that this problem was fixed in C++11 (if I understand correctly). See e.g. https://sourceware.org/bugzilla/show_bug.cgi?id=15366. Have you tried compiling with C++11?
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.
Hello! I'm happy to see that my pull request has been reviewed. In my case SOEM has been included as a submodule to the project on C++17 (compiler: MSVC2019). And i had this issue. Now I haven't got access to this project :( Also I found question on SO about macroses https://stackoverflow.com/a/986584. So, i think this line is neccesary to work properly with C++ compilers. I will fix you other remarks as soon as i can
Add
__STDC_LIMIT_MACROS
and__STDC_CONSTANT_MACROS
compile definitions for work correctly with C++ code (in case when adding library usingadd_subdirectory
from C++ cmake project)Add wpcap files to soem include list. Without it can't find <pcap.h> and other wpcap headers.