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

Thu Feb 16 00:06:30 PST 2012

http (or https) repository enabled would be fine!
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)
Thanks for applying the patches
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.org
> [mailto:nice-bounces+filippo.dellabetta=telecomitalia.it at lists.freedes
> 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.