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

Some addition for cmake #596

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

KomarDL
Copy link

@KomarDL KomarDL commented Feb 25, 2022

Add __STDC_LIMIT_MACROS and __STDC_CONSTANT_MACROS compile definitions for work correctly with C++ code (in case when adding library using add_subdirectory from C++ cmake project)

Add wpcap files to soem include list. Without it can't find <pcap.h> and other wpcap headers.

@nakarlsson nakarlsson requested a review from hefloryd May 24, 2022 09:22
@nakarlsson
Copy link
Contributor

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)
Copy link
Contributor

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)
Copy link
Contributor

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)
Copy link
Contributor

@hefloryd hefloryd Oct 3, 2022

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)
Copy link
Contributor

@hefloryd hefloryd Oct 3, 2022

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?

Copy link
Author

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

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