Skip to content

Add FFI::LastError.win_error#633

Merged
larskanis merged 3 commits intoffi:masterfrom
graywolf:add_win_error
Jan 6, 2019
Merged

Add FFI::LastError.win_error#633
larskanis merged 3 commits intoffi:masterfrom
graywolf:add_win_error

Conversation

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

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

Comment thread ext/ffi_c/LastError.c
#endif

#if defined(_WIN32) || defined(__CYGWIN__)
typedef uint32_t DWORD;
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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.

4 participants