search

Beckhoff / ADS

Beckhoff protocol to communicate with TwinCAT devices.
MIT License
515 stars 197 forks source link

cmake: Make Windows's winsock2 link libraries a public dependency #155

Closed marcus-sonestedt closed 2 years ago

marcus-sonestedt commented 2 years ago

This fixes linking of the executables (adstool, example), as well as any other consumers of adstool, even outside this repo, on Windows.

pbruenn commented 2 years ago

@miq @simonschmeisser any comments on this? If not, I will merge it in the next few days and prepend a "cmake: " prefix to the commit message subject line

miq commented 2 years ago

Ok for me.

simonschmeisser commented 2 years ago

Setting link libraries globally is bad style. In #154 we already added this as a dependency on libads which should be applied transitively to all consumers of libads. Maybe @marcusl could add a PR description stating which error he wants to fix?

marcus-sonestedt commented 2 years ago

@simonschmeisser Ah. I got the errors for the tools & example executables, and the libs for adslib are linked with PRIVATE. Maybe we only need to change that to PUBLIC to make it transitive (since the lib is static, any dependencies on other libs need to be public).

I'll test that and update this PR. Thanks for the quick feedback everyone!

marcus-sonestedt commented 2 years ago

Marking it as public worked. Not sure why I didn't have that change in my repo though, but diff looks ok now.

pbruenn commented 2 years ago

Any comments on the latest change? @marcusl maybe it helps if you rebase this on latest master

marcus-sonestedt commented 2 years ago

The diff looks small enough, and no conflicts, so should be good for a squash-merge. But I can rebase if you think it helps.

marcus-sonestedt commented 2 years ago

rebased on latest master

cowo78 commented 2 years ago

I don't know why wsock32 library is needed but ws2_32 should be required on win32 regardless of the compiler being used.

simonschmeisser commented 2 years ago

in #154 it was stated that it's only needed with msvc but I have no further knowledge or interest on this exotic platform

marcus-sonestedt commented 2 years ago

in #154 it was stated that it's only needed with msvc but I have no further knowledge or interest on this exotic platform

I suppose the compiler (MSVC) was used as the discriminator between Windows and Unix, rather than some OS modifier. (I'm no cmake wizard, yet. ;-) )

We're using Windows temporarily since TwinCAT/NCI is not yet available for TwinCAT/BSD.

pbruenn commented 2 years ago

merged as https://github.com/Beckhoff/ADS/commit/7796fc657f9ad7383501fa3e27f403a3f7f75ba3