-
Notifications
You must be signed in to change notification settings - Fork 337
Add FFI::LastError.win_error #633
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
Conversation
Under cygwin both errno and GetLastError are needed. This adds second getter named #win_error: Linux FFI::LastError.error == errno FFI::LastError.win_error == NoMethodError Windows FFI::LastError.error == GetLastError FFI::LastError.win_error == GetLastError Cygwin FFI::LastError.error == errno FFI::LastError.win_error == GetLastError
Makes sense and LGTM. However I would call the new accessor winapi_error to be more precise. |
ext/ffi_c/LastError.c
Outdated
@@ -49,8 +49,17 @@ | |||
# define USE_PTHREAD_LOCAL | |||
#endif | |||
|
|||
#if defined(_WIN32) || defined(__CYGWIN__) | |||
typedef uint32_t DWORD; |
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.
Why do you declare these functions? This shouldn't be necessary and breaks the MINGW build: https://ci.appveyor.com/project/larskanis/ffi-aofqa/build/1.0.30
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'm stupid, this should obviously be only on cygwin, fixed
I don't have strong opinion on the name either way, so I've renamed it. |
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. @tduehr OK for you as well? Although JRuby doesn't work on Cygwin, winapi_error
should possibly be added to JRuby later on for API compatibility on Windows.
Looks pretty good. I'm not sure about the Linux Between api compat and the Linux subsystem in win10, this will need to make it into JRuby as well. |
Would it be better to leave |
@tduehr Makes sense. But |
Yeah... I think there may be no proper (Ruby-like) solution here... We'll also need to make similar changes to |
So... should I change to pull request to add |
Hello guys, any news on this? I've checked that under Windows (MinGW), FFI.errno (nor FFI::LastError.error) calls aren't accurate. UPDATE: FYI, I've made a workaround from my C code to bypass this problem: // use SET_ERRNO(x) instead of errno=(x) to set errno
#ifdef __MINGW32__
#include <afxres.h>
#define SET_ERRNO(x) SetLastError(x)
#else
#define SET_ERRNO(x) errno=(x)
#endif Using that code, from Ruby FFI, I successfuly retrieve the correct errno. Still, would be nice to have FFI handle this problem (especially for cases where you're using C third hand libs), and I'm not sure how possible this is, so far this thread looked promising. |
To finally solve this issue I merged it as proposed by @graywolf . It is now released as ffi-1.10.0. I would greatly appreciate if someone can take the time to write some simple tests for the new call |
@IgorJorobus could you try to take a look and write some tests as @larskanis asks? Sadly I no longer have a system with cygwin readily available. |
Hi guys, I hope that this specs cut the cake for you. Honestly, I couldn't run the them, just coded them. The rake tasks ask for several things that are missing in my system, isn't enough to just clone the repo, I've tried to fulfill rake needs but couldn't during the time I set for this task. Let me know if I can do something else for you. |
Under cygwin both errno and GetLastError are needed. This
adds second getter named #win_error:
Fixed #632