try a windows build#33
Conversation
|
@kevinkreiser You might like #34 😄 |
|
@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? |
|
@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? |
|
@rbsheth ive cherry picked your commits into this pr. with any luck i can get CI working for windows in the next little while |
|
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) |
|
@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. |
rbsheth
left a comment
rbsheth
left a comment
There was a problem hiding this comment.
I'm guessing you can get away with using only CMake here.
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 and let the lib consumer set them? |
Co-authored-by: Rahul Sheth <rbsheth@cs.stanford.edu>
Co-authored-by: Rahul Sheth <rbsheth@cs.stanford.edu>
Co-authored-by: Rahul Sheth <rbsheth@cs.stanford.edu>
|
@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. |
Co-authored-by: Rahul Sheth <rbsheth@cs.stanford.edu>
|
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 |
|
|
||
| StatsdClient(const StatsdClient&) = delete; | ||
| StatsdClient& operator=(const StatsdClient&) = delete; | ||
| StatsdClient(StatsdClient&&) = default; |
There was a problem hiding this comment.
fixes a warning in windows builds
There was a problem hiding this comment.
turns out that was only a warning when i turned on /Wall which was way too far of a level to turn on anyway so i can just remove it
|
@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. |
|
@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 |
What's the most straightforward setup to test this (when you run on linux)? we could add this to the README |
|
@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? |
👍 |
3 participants
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
ifdefsfor windows but im still messing around with getting the build to work on CI.