-
Notifications
You must be signed in to change notification settings - Fork 77
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
Use windows CI build #204
Use windows CI build #204
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #204 +/- ##
=======================================
Coverage 73.42% 73.42%
=======================================
Files 7 7
Lines 414 414
Branches 68 68
=======================================
Hits 304 304
Misses 68 68
Partials 42 42
Flags with carried forward coverage won't be shown. Click here to find out more. |
bc6f224
to
60f5619
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot draw a clear result from your commit history: Are the ALL_CAPS enum values causing problems with MSVC? Shouldn't this make problems in many other places, as well?
Sorry for my scarce comments on this. Exactly, the all caps ENUMS make strange errors like "missing braces". I found some comments online that on on windows there are lots of macros automatically included for historical reasons. I was not able to see where they are defined and if there is a add_compile_definitions macro in deactivating it. (again, no local possibility, and debugging this in the github runner is not fun). |
@SENAI-GilmarCorreia could you have a look maybe, how to fix this properly? |
@christophfroehlich I am trying to understand the problem here. Do you want to revert the enums back to capital letters? As I have seen, ERROR already exists in Windows systems because this identifier is reserved for error codes in Windows. You can simply rename it by changing the case, as you did, or you can change the name of the enum: enum class return_type : std::uint8_t {
OK = 0,
FAILURE = 1,
DEACTIVATE = 2,
}; I'm testing here as well. |
Thanks for reaching out. Exactly, if only the ERROR is a problem then I'd rename it as you suggested. Or maybe there is a compile definition to avoid this, like NOMINMAX ;) |
As I've seen, this definition is really important. You can use #undef ERROR, but it might cause some unexpected behaviors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine with me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now
@Mergifyio backport humble |
✅ Backports have been created
|
* Use windows CI build * WIN32 CMake changes * Add CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS * Rename ERROR->FAILURE for win builds (cherry picked from commit 252b136)
The main purpose is to avoid adding code which does not compile on Windows. Github provides windows-2019 runners, which can be used here. However, https://github.com/nektos/gh-act does not support this and I was only to test the workflow online.