search

vthiery / cpp-statsd-client

A header-only StatsD client implemented in C++
MIT License
51 stars 19 forks source link

try a windows build #33

Closed kevinkreiser closed 3 years ago

kevinkreiser commented 3 years ago

this is a work in progress. i'm trying to add CI step that builds the library and tests it on windows. with @rbsheth 's help we've updated the library to have the right ifdefs for windows but im still messing around with getting the build to work on CI.

rbsheth commented 3 years ago

@kevinkreiser You might like #34 😄

kevinkreiser commented 3 years ago

@rbsheth looks like you are further along than i. i was starting with the CI bits while you seem to be starting off with the actual code/build changes. do you plan on adding a github action to CI?

rbsheth commented 3 years ago

@kevinkreiser No, these are changes we use internally that have been tested to work cross-platform. You can build on my PR with a Github Action to ensure no breakage?

kevinkreiser commented 3 years ago

@rbsheth ive cherry picked your commits into this pr. with any luck i can get CI working for windows in the next little while

vthiery commented 3 years ago

If you don't mind, I'd prefer to first add the support for Windows and then revisit the topic of Hunter (I have no idea how this manager works neither how it should be configured)

rbsheth commented 3 years ago

@vthiery The impact on non-Hunter users is zero since I've disabled it by default. https://github.com/vthiery/cpp-statsd-client/pull/33/files#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR10

The argument is only needed since Hunter manages the C++ standard/other flags across all compile units.

vthiery commented 3 years ago

@vthiery The impact on non-Hunter users is zero since I've disabled it by default. https://github.com/vthiery/cpp-statsd-client/pull/33/files#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR10

The argument is only needed since Hunter manages the C++ standard/other flags across all compile units.

It doesn't harm indeed but I'm skeptical about having to change a library (or its build config) to have a package manager working. Actually, shouldn't we avoid setting these 

set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_EXTENSIONS OFF)

and let the lib consumer set them?

rbsheth commented 3 years ago

@vthiery Yes, that would be ideal. But setting those is one way to communicate to consumers that the library needs C++11 support. Another way is to set per-target requirements https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html#prop_tgt:CXX_STANDARD. I would suggest setting these on test/other executables and refraining from setting on the library.

kevinkreiser commented 3 years ago

getting close now, should i remove the hunter change for the time being so as not to couple it to windows support? then we can pr it separately?

kevinkreiser commented 3 years ago

haha i had to get a windows vm and install visual studio to actually get this working but it seems to pass now. ill clean it up and ping back for review

kevinkreiser commented 3 years ago

@vthiery this is ready for review! windows CI is up and running (though much slower than linux) and the tests are passing on both platforms with maximal (but still sane) warnings turned on.

kevinkreiser commented 3 years ago

@vthiery this is ready for review, let me know if i can make that easier in any way, i know its a lot

vthiery commented 3 years ago

@vthiery this is ready for review, let me know if i can make that easier in any way, i know its a lot

Thanks for the work and the many comments @kevinkreiser ! I'll try to review this one asap

vthiery commented 3 years ago

haha i had to get a windows vm and install visual studio to actually get this working

What's the most straightforward setup to test this (when you run on linux)? we could add this to the README

kevinkreiser commented 3 years ago

@vthiery im not exactly sure what you mean by: "when you run on linux". do you mean how to check windows builds work on linux? for me i created a virtual box vm of windows and downloaded visual studio and basically ran it on windows natively that way. it will be the same for any windows developer. you simply open the project directory as a cmake project in visual studio and run the test target and it just works. do you want me to document that in the readme?

the comment that you quoted was me laughing at myself for thinking i could debug the windows build via github actions CI logs. in the end it was just too slow to do it that way. so instead of taking forever i just installed a vm and worked in windows directly.

does that make sense?

vthiery commented 3 years ago

i created a virtual box vm of windows and downloaded visual studio and basically ran it on windows natively

:+1: