Skip to content
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

Add FFI::LastError.win_error #633

Merged
merged 3 commits into from Jan 6, 2019
Merged

Add FFI::LastError.win_error #633

merged 3 commits into from Jan 6, 2019

Conversation

graywolf
Copy link
Contributor

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

Fixed #632

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
@larskanis
Copy link
Member

Makes sense and LGTM. However I would call the new accessor winapi_error to be more precise.

@@ -49,8 +49,17 @@
# define USE_PTHREAD_LOCAL
#endif

#if defined(_WIN32) || defined(__CYGWIN__)
typedef uint32_t DWORD;
Copy link
Member

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

Copy link
Contributor Author

@graywolf graywolf May 30, 2018

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

@graywolf
Copy link
Contributor Author

I don't have strong opinion on the name either way, so I've renamed it.

Copy link
Member

@larskanis larskanis left a 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.

@tduehr
Copy link
Member

tduehr commented May 31, 2018

Looks pretty good. I'm not sure about the Linux win_error raising but Windows error not... Not sure what the Windows expectation is here as I work in *nix almost exclusively.

Between api compat and the Linux subsystem in win10, this will need to make it into JRuby as well.

@larskanis
Copy link
Member

@tduehr It would have been better to not unify errno and GetLastError to error. It seemed logical to me as well - until I though about Cygwin. But now it's there and to keep backward compatibility, I think it's best solved like @graywolf proposed.

@tduehr
Copy link
Member

tduehr commented May 31, 2018

Would it be better to leave error alone, and add last_error and errno to split the two interfaces?

@larskanis
Copy link
Member

@tduehr Makes sense. But last_error is too imprecise. I would call it winapi_error or maybe GetLastError like the C function.

@tduehr
Copy link
Member

tduehr commented May 31, 2018

Yeah... I think there may be no proper (Ruby-like) solution here... We'll also need to make similar changes to FFI::Errno. Let's go with GetLastError as it'd be more immediately recognized by windows devs.

@graywolf
Copy link
Contributor Author

graywolf commented Jun 3, 2018

So... should I change to pull request to add LastError#GetLastError? GetLastError is not very ruby-like name for method, so get_last_error?

@damian-m-g
Copy link

damian-m-g commented Jan 2, 2019

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.

@larskanis larskanis merged commit d7d642d into ffi:master Jan 6, 2019
@larskanis
Copy link
Member

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 FFI::LastError.win_error and FFI::LastError.win_error=.

@graywolf graywolf deleted the add_win_error branch January 6, 2019 18:50
@graywolf
Copy link
Contributor Author

graywolf commented Jan 6, 2019

@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.

@damian-m-g
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants