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

Windows: add dependency on ext/sockets #15

Merged
merged 3 commits into from
Mar 12, 2017
Merged

Conversation

Jan-E
Copy link

@Jan-E Jan-E commented Mar 11, 2017

Tested for PHP 7.0.17RC1, x64 nts. I did not run the tests yet, but php_libevent.dll compiled OK with libevent 2.1.6-beta.

@ichiriac
Copy link

Hi @Jan-E,

Not sure to understand why the sockets extension is a requirement on this library (it also could works only on file handles, or just using timer functions). Any reason ?

@Jan-E
Copy link
Author

Jan-E commented Mar 11, 2017

Without it, you get this error at compile time:

libevent.obj : error LNK2019: unresolved external symbol php_sockets_le_socket referenced in function _php_bufferevent_errorcb
C:\php-sdk\php70dev\x64\Release\php_libevent.dll : fatal error LNK1120: 1 unresolved externals
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\BIN\x86_amd64\link.exe"' : return code '0x460'
Stop.

In the config.m4 it is also a non-optional dependency:
https://github.com/expressif/pecl-event-libevent/blob/master/config.m4#L49

If you want it to be optional, you should introduce an AC_DEFINE HAVE_LIBEVENT_SOCKETS in the config.m4/w32 and surround all the code that uses sockets by

#IFDEF HAVE_LIBEVENT_SOCKETS
code
#ENDIF

@ichiriac
Copy link

Many thanks, I've did not saw this.

In fact, the author had already defined something for this : https://github.com/expressif/pecl-event-libevent/blob/master/libevent.c#L688 LIBEVENT_SOCKETS_SUPPORT

Could you help me by making socket dependecy optional, in principle the code was make in order to avoid this depedency.

@Jan-E
Copy link
Author

Jan-E commented Mar 11, 2017

I will take a look at it this evening (CET).

@ichiriac
Copy link

many thanks

@ichiriac ichiriac merged commit a57970b into expressif:master Mar 12, 2017
@ichiriac
Copy link

thanks, it's merged

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.

2 participants