Beckhoff / ADS

Beckhoff protocol to communicate with TwinCAT devices.
MIT License
493 stars 194 forks source link

Fix link error msvc 2019 #154

Closed MelisWillem closed 2 years ago

MelisWillem commented 2 years ago
pbruenn commented 2 years ago

Can someone of the cmake users confirm this change? e.g. @miq @simonschmeisser ?

simonschmeisser commented 2 years ago

Possibly this should check for "Windows" instead of "msvc" but I'm no expert on Windows related things. Thankfully ...

Also does this need to be a public dependency or is private sufficient?

Finally the inside of the conditional block should be indented (2 spaces I think)

MelisWillem commented 2 years ago

@simonschmeisser added the 2 spaces, and put it as private.

MelisWillem commented 2 years ago

@simonschmeisser "MSVC" seems correct according to the cmake docs: https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_COMPILER_ID.html

simonschmeisser commented 2 years ago

@simonschmeisser "MSVC" seems correct according to the cmake docs: https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_COMPILER_ID.html

What i was trying to say is that you could also be using gcc or llvm with the windows api and would probably need the same linker flags. But unless someone comes along with that issue I guess this is fine

MelisWillem commented 2 years ago

@simonschmeisser as far as I know this is MSVC specific. g++ won't have this issue, nor will clang++. Even things like math need to be linked with MSVC and not with clang++ or g++. But I don't have g++ installed under windows at this time.

Also I have no merge rights, so I can't merge this myself..