[Nice] R: R: R: various patches for Windows / Visual Studio compilation

Fri Feb 17 15:49:48 PST 2012

Ah you're right, I didn't think of nice_address...
I definitely don't want to be doing the startup/cleanup in each nice_address
function, so I suggest dropping this and doing the startup/cleanup in the unit
test. I don't see a need for a nice_agent_win32_init though since it's just
about WSAStartup.
If you agree, I will commit the patch that you sent me last time which added
WSAStartup/Cleanup to the unit tests.

Thanks,
Youness

On 02/17/2012 06:00 AM, Della Betta Filippo wrote:
> The problem is that nice_address_set_from_string for instance also fail in WSAStartup is not called.
> In test.c the test fails BEFORE nice_agent_new.
> That's why is not enough to call startup in the init of NiceAgent.
> You'll have to "protect" every API now, and remember to "protect" them in the future..
> Regards,
> Filippo Della Betta
> 
> -----Messaggio originale-----
> Da: nice-bounces+filippo.dellabetta=telecomitalia.it at lists.freedesktop.org [mailto:nice-bounces+filippo.dellabetta=telecomitalia.it at lists.freedesktop.org] Per conto di Youness Alaoui
> Inviato: venerdì 17 febbraio 2012 11:14
> A: nice at lists.freedesktop.org
> Oggetto: Re: [Nice] R: R: various patches for Windows / Visual Studio compilation
> 
> On 02/16/2012 03:06 AM, Della Betta Filippo wrote:
>> http (or https) repository enabled would be fine!
> Going to talk to the sysadmin about it.
> 
>> g_debug is itself a macro, double pass of preprocessor with __VA_ARGS__ could be the problem. Still investigating anyway...
>> Regarding WSAStartup/WSACleanup, I think you'd better add nice_win32_init and nice_win32_deinit and document it to be call before using libnice library under Windows.
>> This is simple to explain, and does not introduce weird dependencies
>> (you have always to create NiceAgent class even if you are not on
>> Windows)
> 
> I wouldn't agree with that because it makes the library work differently depending on whether or not you're on windows, it also would be considered an API break.
> The WSAStartup/WSACleanup as an internal reference count, so if you call startup in the init of the NiceAgent and call cleanup in the finalize, that wouldn't cause any problems, you call nice_agent_new, it calls startup, you g_object_unref the agent, it calls cleanup, and it's all transparent to the user.
> 
>> Thanks for applying the patches
> You're welcome, and thanks for sending them! :)
> 
>> Regards,
>> Filippo Della Betta
>>
>> -----Messaggio originale-----
>> Da: Youness Alaoui [mailto:youness.alaoui at collabora.co.uk]
>> Inviato: mercoledì 15 febbraio 2012 22:36
>> A: Della Betta Filippo
>> Cc: 'nice at lists.freedesktop.org'
>> Oggetto: Re: R: [Nice] various patches for Windows / Visual Studio
>> compilation
>>
>> On 02/15/2012 06:19 AM, Della Betta Filippo wrote:
>>> All other patches.
>> Thank you, I've applied them all apart from 0006, I'll explain why inlined lower.
>>
>>> Last two patches (0007 and 0008) are new and are fixing warnings on Visual Studio compiler.
>> Good, I wonder why I don't see any of these warnings, I thought libnice was getting compiled with the stricted rules.
>> I modified 0008 to actually use the remote's priority (which is
>> already a
>> uint32) instead of the pair's priority.
>>
>>> Regarding __func__ I replaced with the more standard G_STRFUNC and
>>> submitted bug to glib
>>> (https://bugzilla.gnome.org/show_bug.cgi?id=670128)
>> Good, I think that's the better solution.
>>
>>> I put definition of EWOULDBLOCK on pseudotcp.h
>> Good, I modified that patch too in order to add the other error
>> definitions into the .h (ECONNRESET, etc...) and removed the
>> definitions from pseudotcp.c
>>
>>> I'm preparing vcproj and sln for submission Regards, Filippo Della
>>> Betta
>>>
>>> -----Messaggio originale-----
>>> Da: Della Betta Filippo
>>> Inviato: mercoledì 15 febbraio 2012 11:01
>>> A: 'Youness Alaoui'; 'nice at lists.freedesktop.org'
>>> Oggetto: R: [Nice] various patches for Windows / Visual Studio
>>> compilation
>>>
>>> First patch, let me know if it is the right format and indentation
>>> Thanks, Filippo Della Betta
>> Looks good, thanks.
>>
>>>
>>> -----Messaggio originale-----
>>> Da: Della Betta Filippo
>>> Inviato: mercoledì 15 febbraio 2012 09:54
>>> A: 'Youness Alaoui'; nice at lists.freedesktop.org
>>> Oggetto: R: [Nice] various patches for Windows / Visual Studio
>>> compilation
>>>
>>> Asnwers inline
>>> Thanks,
>>> Filippo Della Betta
>>>
>>> -----Messaggio originale-----
>>> Da:
>>> nice-bounces+filippo.dellabetta=telecomitalia.it at lists.freedesktop.or
>>> nice-bounces+g
>>> [mailto:nice-bounces+filippo.dellabetta=telecomitalia.it at lists.freede
>>> s ktop.org] Per conto di Youness Alaoui
>>> Inviato: martedì 14 febbraio 2012 19:17
>>> A: nice at lists.freedesktop.org
>>> Oggetto: Re: [Nice] various patches for Windows / Visual Studio
>>> compilation
>>>
>>> On 02/14/2012 09:11 AM, Della Betta Filippo wrote:
>>>> Hi,
>>> Hi
>>>
>>>>
>>>> attached you can find some patches that issue some Windows / Visual
>>>> Studio compilation problem
>>>>> Thanks, however, like I asked Rohan and Madaro, could you make those git patches, as it makes it easier to apply, also, separate each patch into its own commit.
>>> Ok, can anyone suggest me how to do that ? I'm quite new to git. I've tried git format-patch but I didn't succeed.. Should I make a branch ?
>>> Moreover, is it possible to reach the git repository via http ? we have a http proxy that allows only http/https..
>> Sure, you can use 'git add -p' then select which hunks you want to commit, then you do a git commit, and enter your commit message (first line is a summary, following lines to add more details, try to fit them inside 80 chars per line, makes it easier for viewing).
>> Then you can do 'git format-patch origin/master' to get a patch for every new commit that isn't present in the origin/master branch.
>> Good question about http, it doesn't seem like it's possible with the collabora git server. Do you want me to ask the sysadmin to enable it ?
>>
>>>
>>>>
>>>> In particular
>>>>
>>>> -          On file agent.c and tcp-bsd.c , as glib documentation suggests
>>>> (http://developer.gnome.org/glib/2.28/glib-IO-Channels.html),
>>>> g_io_channel_unix_new should be avoided. Replaced with
>>>> g_io_channel_win32_new_socket function on WIN32 platform
>>>>> Good, but you broke the indentation in the file, please fix it to be indented like the rest of the file.
>>> Ok, I'll do that
>>>
>>>>
>>>> -          On file pseudotcp.c, since Visual Studio compiler does not support
>>>> C99 I put some declarations at the beginning of the statement.
>>>> Moreover I expanded the macro g_debug to make Visual Studio happy. I
>>>> tested it on mingw/msys and seems ok
>>>>> Good catch, but like before, you broke the indentation in the file.
>>>>> However, I don't understand why you had to expand the g_debug macro, you should not have to do that.
>>> I think it's a Visual Studio compiler problem regarding the __VA_ARGS__ macro, with g_debug it doesn't compile...
>>> Maybe someone has a better idea how to fix that..
>> I didn't commit that, it just seems weird because even using g_log, you are still using __VA_ARGS__, so I don't see why it would fail and I'd prefer to keep the use of g_debug rather than hack around it and use g_log.
>> What error do you get exactly? Maybe we can find
>>>
>>>>
>>>> -          On file sha1.h, I included the replacement header win32_common.h
>>>> instead of the missing header stdint.h (at least on Visual Studio).
>>>>> Good
>>>
>>>>
>>>> -          On file win32_common.h I added , if not defined, typedef on size_t
>>>> and ssize_t that are not defined on Visual Studio platform, I tested
>>>> it on mingw/msys as well and seems ok
>>>>> Good
>>>
>>>>
>>>> -          On files test-*.c : most of the time WSAStartup is missing (causing
>>>> test to fail immediately) , on test-pseudotcp.c EWOULDBLOCK is not defined.
>>>> Moreover __func__ is missing and is replaced by __FUNCTION__
>>>>> Good point about WSAStartup, not sure though if those should be put in the tests or if it should be in libnice. Technically libnice does it when it discovers the interfaces, however those >>unit tests are forcing the use of 127.0.0.1, that's why it's not used, so maybe it's ok to leave it in the unit tests. In which case, it would be a good thing to add a note to the >>nice_agent_add_local_address documentation explaining that if it's used, then WSAStartup should be used on windows.
>>> For my experience, WSAStartup is usually put on a "init" library
>>> function and WSACleanup on "deinit" library function, otherwise you
>>> could add a note to every function that use socket that WSAStartup
>>> should be called first
>> Humm.. you're right, I think it's best to call the WSAStartup in the init of the NiceAgent class, and call WSACleanup in the finalize That's why I didn't apply the patch that added WSAStartup to the unit tests.
>> the code in interfaces.c also needs to be fixed since that code doesn't depend on NiceAgent, it means it can be called before NiceAgent is created, so it must do the Startup/Cleanup on its own.. it already calls WSAStartup, but it doesn't call WSACleanup, so this needs to be fixed too.
>>
>>>
>>>>> Also, why do you do :
>>>>> +#ifdef G_OS_WIN32
>>>>> +  WSADATA w;
>>>>> +#endif
>>>>> +
>>>>> +#ifdef G_OS_WIN32
>>>>> +  WSAStartup(0x0202, &w);
>>>>> +#endif
>>>>>
>>>>> Instead of doing :
>>>>> +#ifdef G_OS_WIN32
>>>>> +  WSADATA w;
>>>>> +
>>>>> +  WSAStartup(0x0202, &w);
>>>>> +#endif
>>>
>>> Only to separate declaration from code.. In this case is the same
>>>
>>>>> I don't understand though why you replaced G_STRFUNC with __func__ since that glib macro should take care of it and should be defined properly depending on your compiler.
>>> G_STRFUNC doesn't work for me (it prints ??? instead the name of the function), I will look more in detail to see how to make it work on Windows..
>>> Anyway in other test you use only __func__, that's why I redefined
>>> __func__
>>>
>>>>> About the EWOULDBLOCK, I wonder if pseudotcp.h shouldn't have those defines instead, so every user of the lib wouldn't need to define them. What do you think?
>>> I think it's ok!
>>>
>>>>
>>>>
>>>>
>>>> If you are interested, I can provide you with vcproj and/or sln
>>>> files for Visual Studio 2008 (as well as config.h specific for
>>>> Visual
>>>> Studio)
>>>>> Yes, I think it would be a good idea to put those files under a win32 directory, maybe with a README.win32 file if you think it's necessary.
>>> Ok!
>>>
>>>
>>>>
>>>> Regards,
>>>>
>>>> Filippo Della Betta
>>>>
>>>>
>>> Thank you,
>>> Youness.
>>>
>>>>
>>>> Questo messaggio e i suoi allegati sono indirizzati esclusivamente
>>>> alle persone indicate. La diffusione, copia o qualsiasi altra azione
>>>> derivante dalla conoscenza di queste informazioni sono rigorosamente
>>>> vietate. Qualora abbiate ricevuto questo documento per errore siete
>>>> cortesemente pregati di darne immediata comunicazione al mittente e di provvedere alla sua distruzione, Grazie.
>>>>
>>>> /This e-mail and any attachments// is //confidential and may contain
>>>> privileged information intended for the addressee(s) only.
>>>> Dissemination, copying, printing or use by anybody else is
>>>> unauthorised. If you are not the intended recipient, please delete
>>>> this message and any attachments and advise the sender by return
>>>> e-mail, Thanks./
>>>>
>>>> *rispetta l'ambienteRispetta l'ambiente. Non stampare questa mail se
>>>> non è
>>>> necessario.*
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> nice mailing list
>>>> nice at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/nice
>>>
>>>
>>>
>>> Questo messaggio e i suoi allegati sono indirizzati esclusivamente alle persone indicate. La diffusione, copia o qualsiasi altra azione derivante dalla conoscenza di queste informazioni sono rigorosamente vietate. Qualora abbiate ricevuto questo documento per errore siete cortesemente pregati di darne immediata comunicazione al mittente e di provvedere alla sua distruzione, Grazie.
>>>
>>> This e-mail and any attachments is confidential and may contain privileged information intended for the addressee(s) only. Dissemination, copying, printing or use by anybody else is unauthorised. If you are not the intended recipient, please delete this message and any attachments and advise the sender by return e-mail, Thanks.
>>>
>>
>>
>>
>> Questo messaggio e i suoi allegati sono indirizzati esclusivamente alle persone indicate. La diffusione, copia o qualsiasi altra azione derivante dalla conoscenza di queste informazioni sono rigorosamente vietate. Qualora abbiate ricevuto questo documento per errore siete cortesemente pregati di darne immediata comunicazione al mittente e di provvedere alla sua distruzione, Grazie.
>>
>> This e-mail and any attachments is confidential and may contain privileged information intended for the addressee(s) only. Dissemination, copying, printing or use by anybody else is unauthorised. If you are not the intended recipient, please delete this message and any attachments and advise the sender by return e-mail, Thanks.
>>
>> _______________________________________________
>> nice mailing list
>> nice at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/nice
> 
> 
> 
> Questo messaggio e i suoi allegati sono indirizzati esclusivamente alle persone indicate. La diffusione, copia o qualsiasi altra azione derivante dalla conoscenza di queste informazioni sono rigorosamente vietate. Qualora abbiate ricevuto questo documento per errore siete cortesemente pregati di darne immediata comunicazione al mittente e di provvedere alla sua distruzione, Grazie.
> 
> This e-mail and any attachments is confidential and may contain privileged information intended for the addressee(s) only. Dissemination, copying, printing or use by anybody else is unauthorised. If you are not the intended recipient, please delete this message and any attachments and advise the sender by return e-mail, Thanks.
> 
> _______________________________________________
> nice mailing list
> nice at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nice

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/nice/attachments/20120217/ee0e8b5d/attachment-0001.pgp>