qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Cliff Jansen" <cliffjan...@gmail.com>
Subject Re: Review Request: patches for mingw
Date Tue, 07 Aug 2012 17:07:33 GMT


> On Aug. 2, 2012, 2:58 p.m., Kenneth Giusti wrote:
> > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/CMakeLists.txt, line
39
> > <https://reviews.apache.org/r/6302/diff/1/?file=132723#file132723line39>
> >
> >     Actually, I think driver.c should be split in two: there's quite a bit of platform
generic code in this file that should be shared.
> >     
> >     Take a look at the "driver_abstraction" branch upstream.  I've split driver.c
into two files - isolating (some) of the platform code into a separate "impl" file.
> >     
> >     To support windows, we'd have to cleave off more code than I have in the branch,
but it's a start.

I agree.  The current windows/driver.c is a straw man, just to get us rolling.  When the time
comes to do it properly using I/O completion ports (and presumably stick handling the ssl
layer too), I (or any interested party) can continue working with what you have.


> On Aug. 2, 2012, 2:58 p.m., Kenneth Giusti wrote:
> > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger.c, line
324
> > <https://reviews.apache.org/r/6302/diff/1/?file=132732#file132732line324>
> >
> >     The sematics are somewhat different here: 0.0.0.0 == all addresses.  Is this
change ok?

It is necessary to work on Windows, where the OS treats a connection to "all addresses" as
meaningless, and returns an error.

Listening on 0.0.0.0 is fine, and those uses are unchanged.


> On Aug. 2, 2012, 2:58 p.m., Kenneth Giusti wrote:
> > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/proton-dump.c, line
35
> > <https://reviews.apache.org/r/6302/diff/1/?file=132733#file132733line35>
> >
> >     Problem: calling sprintf may change errno, which means perror could print the
wrong description.

Good catch.  will fix.


> On Aug. 2, 2012, 2:58 p.m., Kenneth Giusti wrote:
> > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sys/windows/driver.c,
line 1
> > <https://reviews.apache.org/r/6302/diff/1/?file=132737#file132737line1>
> >
> >     See my previous comment re: splitting up this file.
> >     
> >     I think we should come up with a layered abstraction here, so we can not only
support different platforms (win vs unix), but also different i/o mechs (poll, epoll, windows
completions), and the use of different TLS/SSL impls.

Agreed.  But I would like to get an initial Windows version going first.


> On Aug. 2, 2012, 2:58 p.m., Kenneth Giusti wrote:
> > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/proton-dump.c, line
87
> > <https://reviews.apache.org/r/6302/diff/1/?file=132733#file132733line87>
> >
> >     ditto

ditto


- Cliff


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6302/#review9755
-----------------------------------------------------------


On Aug. 6, 2012, 8:16 p.m., Cliff Jansen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6302/
> -----------------------------------------------------------
> 
> (Updated Aug. 6, 2012, 8:16 p.m.)
> 
> 
> Review request for qpid, Andrew Stitcher, Kenneth Giusti, Steve Huston, and Rafael Schloming.
> 
> 
> Description
> -------
> 
> This patch set works with a recent mingw32, cmake 2.8.1, python 2.5, swig 2.0.7.
> 
> A push-button build is still a ways off.  The custom_commands in the cmake script to
generate the protocol headers don't work yet on Windows.
> 
> The most intrusive change was the introduction of a pn_socket_t type to hold a socket
on both Windows and Posix platforms.  An attempt was made to minimize the use of #ifdefs and
split platform code into separate posix and windows directories, as is done for the C++ code.
 There is so little needed at the moment, this may be overkill.  The qpid-proton-posix library
was ditched and combined with the main qpid-proton library.  Instead, the work is done in
CMake to assemble the correct shared and platform specific sources as is done in the C++ tree.
> 
> The driver.c implementation is proof of concept using Winsock select().  Future work
would most likely replace this with an I/O completion port implementation.
> 
> 
> 1. mkdir ...\trunk\proton-c\build
> 
> 2. set env vars as per trunk\config.sh
> 
> 
>   set PATH=C:\Program Files (x86)\CMake 2.8\bin;C:\python25;C:\mingw_ptn\bin;C:\Windows\System32;c:\cj\work\amqp\proton\mingw4\trunk\proton-c\build
>   set PYTHONPATH=c:\cj\work\amqp\proton\mingw4\trunk\tests;c:\cj\work\amqp\proton\mingw4\trunk\proton-c;c:\cj\work\amqp\proton\mingw4\trunk\proton-c\build\bindings\python
>   set PYTHON_BINDINGS=c:\cj\work\amqp\proton\mingw4\trunk\proton-c\build\bindings\python
>   set PROTON_HOME=C:\cj\work\amqp\proton\mingw4\trunk
> 
> 3. generate the headers:
> 
>   cd trunk\proton-c\build
>   python c:\cj\work\amqp\proton\mingw4\trunk\proton-c\src\codec\encodings.h.py >encodings.h
>   python c:\cj\work\amqp\proton\mingw4\trunk\proton-c\src\protocol.h.py >protocol.h
> 
> 4. build
> 
>   cmake -G "MinGW Makefiles" -DSWIG_EXECUTABLE=C:\swigwin-2.0.7\swig.exe c:\cj\work\amqp\proton\mingw4\trunk\proton-c
>   mingw32-make
>   python ..\..\tests\proton-test
> 
> 
> This addresses bug QPID-4181.
>     https://issues.apache.org/jira/browse/QPID-4181
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/CMakeLists.txt 1369190 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/bindings/CMakeLists.txt
1369190 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/bindings/php/php.i 1369190

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/bindings/python/python.i
1369190 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/include/proton/driver.h
1369190 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/codec/codec.c 1369190

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/dispatcher/dispatcher.c
1369190 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/driver.c 1369190 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/framing/framing.c 1369190

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/message/message.c 1369190

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger.c 1369190

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/proton-dump.c 1369190

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/proton.c 1369190 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sys/network.h PRE-CREATION

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sys/posix/time.c PRE-CREATION

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sys/uuid.h PRE-CREATION

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sys/windows/driver.c
PRE-CREATION 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sys/windows/time.c PRE-CREATION

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sys/windows/uuid.c PRE-CREATION

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/util.h 1369190 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/util.c 1369190 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/tests/proton_tests/messenger.py 1369190

> 
> Diff: https://reviews.apache.org/r/6302/diff/
> 
> 
> Testing
> -------
> 
> Fedora, Windows
> 
> 
> Thanks,
> 
> Cliff Jansen
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message