237870 - MinGW builds against wrong sockets DLL

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[[e-mail]]

MSVC builds link against wsock32.lib but some makefiles specify -lws2_32 as the
MinGW equivalent. This prevents MinGW from building on some platforms.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[[e-mail]]

Comment on attachment 144218 [details] [diff] [review]
Proposed patch

As this touches protected partitions I'd appreciate it if you checked in any
bits to which you agree.

Comment 3

[cls]

On what platforms does this prevent mingw from building on?  ws2_32 is the
winsock2 library; it should be on all boxes.  Besides, it's required to get full
getsockopt functionality.  See
http://bugzilla.mozilla.org/show_bug.cgi?id=134113#c108 .

Comment 4

[[e-mail]]

I'm sorry, but I don't understand why mingw builds link against ws2_32.dll but
msvc builds link against wsock32.dll - the platform I'm trying to build on
doesn't have ws2_32.dll (so far I've had no issues relating to wsock32.dll).

Comment 5

[cls]

The reerenced fcomment in the original mingw bug explains why we need to link
against ws2_32.dll for mingw builds.  I honestly have no idea why msvc doesn't
need to link against the winsock2 dll or even if msvc builds will pass the nspr
tests.  Passing the nspr tests was a requirement for landing mingw support so
here we are.  Have you tested to see if the nspr tests will pass with your changes?

Comment 6

[Wan-Teh Chang]

Neil: is your platform Windows 95?  I know old Windows 95
releases don't have ws2_32.dll.

It is possible that we are not defining the right macros
when we are compiling with MinGW so that when we link
with wsock32.dll it isn't able to use certain socket
options that are only implemented in Winsock 2.

Unfortunately I am not set up to debug a MinGW build.
I hope someone can figure this out.  Yeah, I've also
been wondering why the MinGW build must link with ws2_32.dll
in order to pass all of the NSPR tests.  (By the way,
the MSVC build passes all of the NSPR tests.)

Comment 7

[[e-mail]]

Assuming "make runtests" in nsprpub/pr/tests is the way to go, the tests don't
actually pass for me with the (msvc) mozilla 1.5 release nspr... on the other
hand, with the patch applied, mozilla starts up my home page fine...

Comment 8

[cls]

IIRC, to properly run the tests, you need to: cd pr/tests && sh runtests.ksh

Comment 9

[Wan-Teh Chang]

Yes, "make runtests" is obsolete.  You need to say "sh runtests.ksh".

We have been using the MKS Korn Shell to run runtests.sh.  I don't know
if that shell script works under Cygwin's bash.

Comment 10

[[e-mail]]

MinGW doesn't define IP_TOS or IP_TTL in winsock.h which is a bug in MinGW as
far as incompatibility with msvc is concerned; rather than forcing MinGW to use
ws2tcpip.h would something along these lines would be acceptable? [to replace
the existing #ifdef WIN32 block in prmapopt.c]

#ifdef WIN32
#include <winsock.h>
/* MinGW doesn't define these in its winsock.h */
#ifndef IP_TTL
#define IP_TTL 7
#endif
#ifndef IP_TOS
#define IP_TOS 8
#endif
#endif

Comment 11

[Wan-Teh Chang]

That sounds like a good idea.  Please try this:

1. In prmapopt.c, restore the #ifdef WIN32 block to
what it was before:

#ifdef WINNT
#include <winsock.h>
#endif

2. In nsprpub/pr/include/md/_win95.h, add the following
after the winsock.h inclusion:

/* MinGW doesn't define these in its winsock.h */
#ifdef __MINGW32__
#ifndef IP_TTL
#define IP_TTL 7
#endif
#ifndef IP_TOS
#define IP_TOS 8
#endif
#endif

Comment 12

[[e-mail]]

Attachment: Updated patch to NSPR

(In reply to comment #11)
>That sounds like a good idea.	Please try this:
>1. In prmapopt.c, restore the #ifdef WIN32 block to what it was before:
>#ifdef WINNT
>#include <winsock.h>
>#endif
Did you mean WIN32 ? The sockopt test failed with this changed to WINNT.

Comment 13

[Wan-Teh Chang]

Attachment: Updated patch to NSPR v2

You are right.	I also tried the changes I suggested
and found that they don't work.

Please try this patch instead.

Comment 14

[Wan-Teh Chang]

Comment on attachment 144466 [details] [diff] [review]
Updated patch to NSPR v2

I forgot to mention that this patch is essentially
what Neil suggested in comment 10.

We probably could move the definitions of IP_TTL
and IP_TOS before the "primpl.h" inclusion.

Comment 15

[[e-mail]]

(In reply to comment #14)
>(From update of attachment 144466 [details] [diff] [review])
This patch PASSED too :-)
>We probably could move the definitions of IP_TTL
>and IP_TOS before the "primpl.h" inclusion.
If you do that, you could possibly separate the #ifdef WINNT and #ifdef
__MINGW__ sections each having their own #include <winsock.h> lines.

Comment 16

[Wan-Teh Chang]

Attachment: Updated patch to NSPR v2.1

Neil, thanks for testing NSPR patch v2.  In this
patch I made the change I suggested: define IP_TTL
and IP_TOS before including primpl.h.  (Your suggestion
would also work.  It's mainly a matter of personal
taste.)

Could you tell me which platforms don't have ws2_32.dll?
Current Microsoft documentation tells people to include
winsock2.h and link with ws2_32.dll.  So it is likely
that we will switch to Winsock 2 some time soon.

Comment 17

[cls]

Comment on attachment 144491 [details] [diff] [review]
Updated patch to NSPR v2.1

My mingw build passes the test suite with these changes. r=cls

Comment 18

[[e-mail]]

Comment on attachment 144218 [details] [diff] [review]
Proposed patch

Do you have any issues with the non-nspr changes in this patch?

Comment 19

[Wan-Teh Chang]

Comment on attachment 144218 [details] [diff] [review]
Proposed patch

r=wtc.	The non-NSPR changes in this patch are fine.

Comment 20

[Wan-Teh Chang]

Comment on attachment 144491 [details] [diff] [review]
Updated patch to NSPR v2.1

I've checked in this NSPR patch on the NSPR tip (NSPR 4.5)
and NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.7 final).

Neil, you just need to check in the non-NSPR changes.

Comment 21

[cls]

One of those non-NSPR changes was to
security/nss/lib/fortcrypt/swfort/pkcs11/Makefile .

Comment 22

[Wan-Teh Chang]

Comment on attachment 144218 [details] [diff] [review]
Proposed patch

The security/nss change in this patch has been
checked into the NSS tip (NSS 3.10), NSS_3_9_BRANCH
(NSS 3.9.1), and NSS_CLIENT_TAG (Mozilla 1.7 final).

Comment 23

[[e-mail]]

Checking in Makefile.in;
/cvsroot/mozilla/netwerk/build/Makefile.in,v  <--  Makefile.in
new revision: 1.62; previous revision: 1.61
done

Thanks everybody.