112987 - freebsd ports tree doesn't submit patches to mozilla.org

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect, P3)

Description

[clkao]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:0.9.6+) Gecko/20011201
BuildID:    

recent freebsd nightlys (since one or two weeks ago) stop working. The main
browser frame remains blank even after loading web pages. however, the window
title is still changed according to loaded pages' title. by supplying the
patches reside in freebsd ports collection to latest tree, the newly built
mozilla works properly again. I'm not sure about which part of the patch fixes
the incorrect behavior, but someone may want to review and/or incorporate the
patches from freebsd ports collection

the patches are available at
http://www.freebsd.org/cgi/cvsweb.cgi/ports/www/mozilla/files/


Reproducible: Always
Steps to Reproduce:
1.launch mozilla nightly on freebsd

Comment 1

[timeless]

Attachment: nspr changes from FreeBSD Ports

Comment 2

[timeless]

Attachment: c ldap changes

Comment 3

[timeless]

Attachment: xpcom changes

Comment 4

[timeless]

Attachment: configure changes

Comment 5

[timeless]

wtc: nsprpub+xpcom are for you.
dmose: can you find someone to look at directory 
part of configure is for jcgriggs, it's not a direct match of what ports had.
jcg, I know we can't predict all possible placements of Qt, but apparently it's 
standardized on FreeBSD
cls: the rest is yours

i'm sorry about the non -u's, xpcom is a move (for C rules i guess), and 
directory is a substitution.

reporter: our corp builds mozilla daily on freebsd and we haven't had the 
problem you described.

Comment 6

[clkao]

The freebsd nightly build 2001120207 works great with clean install/preferences.

Comment 7

[hacker formerly known as [e-mail]]

Comment on attachment 60090 [details] [diff] [review]
nspr changes from FreeBSD Ports

I'm not sure why they are bothering to change FreeBSD.mk, it's not used
anymore.  And they are hardcoding -D_REENTRANT & -pthread which wouldn't work
in previous FreeBSD releases.

Comment 8

[hacker formerly known as [e-mail]]

Comment on attachment 60092 [details] [diff] [review]
xpcom changes

r=cls

Comment 9

[hacker formerly known as [e-mail]]

Comment on attachment 60093 [details] [diff] [review]
configure changes

Provide a patch against configure.in instead of configure.  The Qt check
doesn't make any sense as it checks if QTDIR & QTINCDIR are empty immediately
after setting them to non-empty values.  Where does BSD_PTHREAD_LIBS come from?

Comment 10

[hacker formerly known as [e-mail]]

Comment on attachment 60091 [details] [diff] [review]
c ldap changes

r=cls

Comment 11

[timeless]

Comment on attachment 60093 [details] [diff] [review]
configure changes

Ok, next time i'll man test first and guess later.
I think the questionable line  should be:
if test ! -d "$QTDIR$QTINCDIR"; then

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 60091 [details] [diff] [review]
c ldap changes

This particular patch has already been checked in as part of bug 109210.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 60092 [details] [diff] [review]
xpcom changes

diff -u format in the future, please.  And consider splitting bugs by component
affected by the patch.

/be

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Updated patch for configure/configure.in

Updated the configure changes, and made the equivalent changes to configure.in

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

For some reason processmail gave an error about "bug number is not an integer"
when I posted my updated patch for configure (and I added the equivalent changes
to configure.in).  These should be good to go in; I'd made similar changes in a
local tree.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 60090 [details] [diff] [review]
nspr changes from FreeBSD Ports

r=[e-mail]

Comment 17

[hacker formerly known as [e-mail]]

Comment on attachment 62957 [details] [diff] [review]
Updated patch for configure/configure.in

This configure.in patch still seems terminally broken.	You're removing the
appending of -pthread to CFLAGS/CXXFLAGS for the general case.	That's bad.  On
some systems, -pthread adds addtional defines & flags when passed to CC that
should be used in a multi-threaded env.  

What is BSD_PTHREAD_LIBS and why is it being appended to NSPR_LIBS?  Why isn't
NSPR_LIBS being set properly?

The qt test is still wrong. After setting QTDIR & QTINCDIR, checking if the
combined string is empty will always fail.  Or did you mean -d instead of -z ?

Comment 18

[hacker formerly known as [e-mail]]

The xpcom & nspr (-FreeBSD.mk) changes have been checked in.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

My apologies, I was rolling forward those older configure changes posted by
someone else and trying to replicate them in configure.in.  I didn't review them
in great detail.  I'll go back over them with a fine-tooth-comb tomorrow.

Comment 20

[Wan-Teh Chang]

Comment on attachment 60090 [details] [diff] [review]
nspr changes from FreeBSD Ports

>Index: pr/include/md/_freebsd.h
>===================================================================
>RCS file: /cvsroot/mozilla/nsprpub/pr/include/md/_freebsd.h,v
>retrieving revision 3.13
>diff -u -r3.13 _freebsd.h
>--- pr/include/md/_freebsd.h	20 Jun 2000 21:22:19 -0000	3.13
>+++ pr/include/md/_freebsd.h	2 Dec 2001 21:31:39 -0000
>@@ -65,7 +66,7 @@
> #define _PR_HAVE_SOCKADDR_LEN
> #define _PR_STAT_HAS_ST_ATIMESPEC
> #define _PR_NO_LARGE_FILES
>-#if ( __FreeBSD__ > 2 )
>+#if ( __FreeBSD_version >= 220000 ) && ( __FreeBSD_version < 400008 )
> #if !defined(_PR_PTHREADS)
> /*
>  * libc_r doesn't have poll().  Although libc has poll(), it is not

I have questions about this change.  I will try to contact
the author of this patch to find out what problem he was
trying to solve and see if his solution is correct or if
there is a better solution.

Comment 21

[timeless]

Comment on attachment 60092 [details] [diff] [review]
xpcom changes

cls committed as r3.6

Comment 22

[Jeremy Lea]

Arrgh, darn, I just wrote a whole long explaination of all the patches into 
this silly little box, then closed the wrong browser window!

So, anyway, Hi,

Until recently I was the maintainer of mozilla in the FreeBSD Port's 
Collection.  While most of the patches are not mine I will try to explain 
them...  Wan-Teh asked me to explain one of the patches.

These are from http://www.freebsd.org/cgi/cvsweb.cgi/ports/www/mozilla/files/, 
not the attachments to this bug.

patch-Makefile.in: (from [e-mail] - for 0.9.7)
Fix a problem with the $(NULL) not being added at the end of the list...

patch-ak:
For some background on __FreeBSD_version see 
http://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/porting-
versions.html

The magic of 400008 is that a thread safe poll() was added to libc_r.  Looking 
at the patch it shouldn't be 220000, but 300000...  I mischanged the old test.  
But it won't make any difference since mozilla has never compiled on a pre 3.x 
FreeBSD (netscape did, but not mozilla).

patch-aq: (from Peter Haight <[e-mail]>, [e-mail])
Obvious patch to use sched_getpriority_max() instead of a hard coded value.

patch-embedding::config::basebrowser-unix: (from [e-mail] - 0.9.7)
We compile SVG in for fun, so libmoz_art_lgpl needs to be added to the embedded 
config.

patch-extensions::transformiix::source::base::Double.cpp (from 
[e-mail] - 0.9.7)
Correct floating point handling.  FreeBSD does not have a <floatingpoint.h> and 
the Alpha has different FP mask flags.

patch-mi: (from Mikhail Teterin <[e-mail]> - for M10)
Use the system defined C++ compiler (which was eg++ at that time).

patch-nsprpub::pr::include::md::_freebsd.cfg: (from [e-mail] - for 
0.9.7)
Support for the Alpha, which is a 64 bit platform...

patch-qt: (let's work our way through the chunks...)
@@ -6186,6 +6186,9 @@
 os2*)
     LIBS=
     ;;
+freebsd*)
+    CPPFLAGS="${CPPFLAGS} ${X_CFLAGS}"
+    ;;
 esac
 for ac_hdr in sys/byteorder.h compat.h getopt.h
 do

From me.  IIRC, some libraries needed for X were not being passed, and so 
configure was failing to link -lXshm, and so not using shared memory segments...

@@ -7357,8 +7360,6 @@
 	if test $? -eq 0; then
 		if test -z "`egrep -i '(unrecognize|unknown)' conftest.out | 
grep pthread`" && test -z "`egrep -i '(error|incorrect)' conftest.out`" ; then
 			ac_cv_have_dash_pthread=yes
-			CFLAGS="$CFLAGS -pthread"
-			CXXFLAGS="$CXXFLAGS -pthread"
 		fi
 	fi
 	rm -f conftest*
@@ -7392,7 +7393,7 @@
 EOF
 
 						if 
test "$ac_cv_have_dash_pthread" = "yes"; then
-				_PTHREAD_LDFLAGS=""
+				_PTHREAD_LDFLAGS="-pthread"
 			else
 				_PTHREAD_LDFLAGS="-lc_r"
 			fi

-pthread is the offical FreeBSD way of linking threaded binaries.  It is a 
linker flag, not a compiler flag.  I had this arguement with cls some years 
ago, but could never persuade him...  On FreeBSD 3.x and 4.x, libc_r is a 
replacement for libc, on 5.x it is an add on (like libpthread on Linux).  
Using -pthread causes the linker to to the right thing, and this chunk of the 
patch should work on all versions of FreeBSD.

(as a side note:  FreeBSD biniaries should never be explictly linked with -lc, 
since this breaks threading).

@@ -9839,7 +9840,7 @@
     fi
     MOC=$HOST_MOC
 
-    QT_CFLAGS="-I${QTDIR}${QTINCDIR} -DQT_GENUINE_STR"
+    QT_CFLAGS="-I${QTDIR}/include/X11/qt -DQT_GENUINE_STR"
     QT_LIBS="-L/usr/X11R6/lib -L${QTDIR}/lib -lqt -lXext -lX11"
 
     # Check whether --with-static-qt or --without-static-qt was given.

Someone once wanted to try the Qt frontend, so I obliged them, and stuck this 
patch in.  There was more, but it's rotted and gone.  The Port has never used 
Qt, but the jist of the patch was that the Qt Port puts it's files in strange 
places on FreeBSD.  It's basically a FreeBSD Port's only patch...

@@ -13711,6 +13712,7 @@
     echo "configure: warning: Recreating autoconf.mk with updated nspr-config 
output" 1>&2
     if test ! "$VACPP" && test -z "$_WIN32_MSVC"; then
         NSPR_LIBS=`./nsprpub/config/nspr-config --prefix=$MOZ_BUILD_ROOT/dist -
-exec-prefix=$MOZ_BUILD_ROOT/dist --libs`
+	NSPR_LIBS="$NSPR_LIBS $BSD_PTHREAD_LIBS"
         $PERL -pi.bak -e "s {^NSPR_LIBS\s*=.*} {NSPR_LIBS = $NSPR_LIBS }" 
config/autoconf.mk
     fi
     if test -z "$_WIN32_MSVC"; then

BSD_PTHREAD_LIBS is PTHREAD_LIBS, from line 60 of the Port's Makefile 
(http://www.freebsd.org/cgi/cvsweb.cgi/ports/www/mozilla/Makefile?annotate=1.75)
which in turn comes from bsd.port.mk, the magic which drives the ports 
collection. (http://www.freebsd.org/cgi/cvsweb.cgi/ports/Mk/bsd.port.mk.diff?
r1=1.362&r2=1.363)  Once apon a time, someone broke -pthread, and so we have 
$PTHREAD_LIBS, as a legacy. (sobomax also submited the patch to bsd.port.mk, 
and like to use it...).  This is the wrong fix.  The correct one is to have 
nspr-config include the correct linker flags (i.e. add -pthread).

patch-xpcom::ds::plvector.c: (from [e-mail] - 0.9.7)
Don't understand this patch, it appears to be a no op...

patch-xpfe::bootstrap::nsAppRunner.cpp: (from [e-mail] - 0.9.7)
More floating point fixes...

Also, if you look at the Makefile linked above, there are a number of other 
nasty things the Port does.  It disables the user's CFLAGS and CXXFLAGS...  I 
don't know why sobomax did this.  It appears to be a very heavy handed way of 
stopping people using -O6...

It also uses MALLOC_OPTIONS=j, which causes malloc() to prezero all memory it 
returns.  I don't know why this was added, since it will have no effect on the 
final binary, only on the build process.  I think it might have been something 
to work around a crash caused by one of the tools not initializing it's 
memory...

And then there's the post-build target to squash bug 51677.

Hope this helps.
  -Jeremy

PS - is there a way of adding comments via email?

Comment 23

[Wan-Teh Chang]

Hi Jeremy,

Thank you for the explanation of the patches.  By the way,
I noticed the new NSPR _freebsd.cfg patch for FreeBSD/alpha
and have checked it in.

One question -- according to the porting-versions.html page
you cited, we should include <osreldate.h> for the __FreeBSD_version
macro.  It seems that <sys/param.h> is for the BSD macro, which we
don't use.

After reading your explanation and going over the comments
for the original code, I found that your patch does not do
what you intend to do: to use libc_r's poll() in the pthreads
version of NSPR.  I will attach a patch that makes NSPR use
libc_r's poll() or libc's poll() where appropriate.

PS -- there is no way to add comments via email.

Comment 24

[Wan-Teh Chang]

Attachment: NSPR patch, to be applied on top of the NSPR changes from FreeBSD Ports (attachment 60090)

Please review and test my patch for NSPR's _freebsd.h.

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

I have a patch I'll post tomorrow that for configure(.in) that fixes some of
these issues (and patches nsprpub/configure(.in) in the same way).

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

Attachment: updated patch for configure(.in) and nsprpub/configure(.in)

This shouldn't break any other systems, and also fixes nsprpub/configure (ports
didn't have this because it only hits you if you build $SIMPLE_PROGRAMS in
there, which happens if tests are turned on (they aren't in freebsd ports). 
Note: my changes only supress -pthread for CFLAGS/CXXFLAGS under FreeBSD.  I
suspect that this should apply to OpenBSD and BSDI, but I don't think it hurts
to include -pthread on CFLAGS/CXXFLAGS.

Comment 27

[Jeremy Lea]

Wan-Teh, your patch looks good, but I've not had a chance to test it yet.  
Unfortunately I'm a bit short of test boxes at the moment.  So, we've not been 
using poll() this whole time...

<osreldate.h> is the right header to use.  I think I just put <sys/param.h> in 
because that is where I always see __FreeBSD_version changes in the CVS 
logs...  <osreldate.h> is written when you update the system based on the 
contents of <sys/param.h>, and doesn't include all of the pollution from that 
file (like definitions of TRUE and FALSE).  Sometimes it's good to go back and 
look at the docs, rather than just committing the first thing that works ;-)

Randell, your patch looks OK, except that I think you should leave out the Qt 
bits, since no one is using them.  Also, on FreeBSD, QTDIR should 
be "/usr/X11R6".

I'll add an attachment to this bug with the two <floatingpoint.h> fixes.

Regards,
  -Jeremy

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Updated X11 -> X11R6

Updated X11 -> X11R6.  I'm leaving the QT fix in since it causes me build
problems on BSD without it even though I don't use QT.	If you like it, please
either r/sr or check it in.

Comment 29

[Wan-Teh Chang]

Comment on attachment 63328 [details] [diff] [review]
NSPR patch, to be applied on top of the NSPR changes from FreeBSD Ports (attachment 60090 [details] [diff] [review])

Thanks for reviewing my patch, Jeremy.
I went ahead and checked in this patch
on the tip and client branch of NSPR.

Comment 30

[Wan-Teh Chang]

Comment on attachment 63560 [details] [diff] [review]
Updated X11 -> X11R6

Is there any harm to have -pthread on the compile command line?
If -pthread is not a compiler flag, is there any compiler flag
for pthreads?  Do we need to say -D_REENTRANT or -D_THREAD_SAFE,
for example?

Comment 31

[Randell Jesup [:jesup] (needinfo me)]

I'm pretty sure there's no harm, though I can't say 100% for sure.  So, yes, I
think we could lose that part of the patch.  I don't know about the others -
aren't they handled by FreeBSD.mk?

Comment 32

[Jeremy Lea]

Attachment: Revised patches for 0.9.8.

I;ve attached a new set of patches from the FreeBSD 0.9.8 Port.

Running through the file:

Makefile.in:  Fix a logic bug in where the $(NULL) occurs.
configure{|.in}: Previously discussed stuff + revised QT patch.
basebrowser-unix:  Might not be wanted.  Not everyone is english.
Double.cpp: Fix floating point on FreeBSD-alpha.
unixcharset.properties: Add Ukrainian.
nsprpub/configure{|.in}: Link with gcc, and correct pthreads flags.
FreeBSD.mk: Not used, but patch to prevent bad example.
_freebsd.h: Enable IPv6.
nsAppRunner.cpp: FreeBSD doesn't have a <floatingpoint.h>

I've tested these patches, and Mozilla 0.9.8 compiles and runs.

I have not included the other IPv6 patches which have already been merged.

I still need to fix the NSS build system, which is using a bad copy of
FreeBSD.mk.

Please obsolete all of the other patches.

Regards,
  -Jeremy

Comment 33

[Wan-Teh Chang]

Comment on attachment 69365 [details] [diff] [review]
Revised patches for 0.9.8.

I've reviewed all the NSPR changes.

>Index: nsprpub/configure.in

I opened bug 125857 for the changes to this file.

>Index: nsprpub/pr/include/md/_freebsd.h

I received this patch from Munechika Sumikawa
last week and have checked it in. (bug 124981)

Comment 34

[hacker formerly known as [e-mail]]

Comment on attachment 69365 [details] [diff] [review]
Revised patches for 0.9.8.

Not sure why the setting of NULL in Makefile.in is considered a logic error. 
It's done all across the tree so that when a directory is added, we don't have
to modify multiple lines.

The basebrowser-unix changes are only used if you are packaging the embedding
tests. You'll have to open a separate bug on that one.	

The rest of the changes look fine.  I'll attach a trimmed down patch since wtc
split off the NSPR changes.

Comment 35

[hacker formerly known as [e-mail]]

Attachment: trimmed patch for 0.9.9

Comment 36

[hacker formerly known as [e-mail]]

Comment on attachment 71362 [details] [diff] [review]
trimmed patch for 0.9.9

r=cls

Comment 37

[Jeremy Lea]

Attachment: Patch for NSS FreeBSD.mk

This is an additional patch which brings security/coreconf/FreeBSD.mk in line
with reality.  I'm still having a problem getting my CFLAGS into the command
line, but that's just being a pedantic FreeBSD porter...

If this needs to be a new bug, just yell.

The revisied patch looks great.  I just attached our full patch set, including
all of the things for embedding/SVG.  We can leave those out of the Mozilla
tree for the time being.

Regards,
  -Jeremy

Comment 38

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 0.9.9

Comment 39

[hacker formerly known as [e-mail]]

A slightly modified attachment 71362 [details] [diff] [review] has been checked in.  I had to separate
freebsd from the openbsd/bsdi switch like we did for nspr.

Yes, you'll have to file a separate bug on NSS for your patch.  As for passing
CFLAGS to NSS, that's a known problem.  See bug 93206 & bug 107976.

Comment 40

[Wan-Teh Chang]

Attachment: Patch for NSS FreeBSD.mk

Jeremy,

I reviewed and edited your patch.  Please review and
test this patch.  Thanks.

Comment 41

[Jeremy Lea]

Wan-Teh, the new patch looks good.  I see that one of the other bugs that
Christopher mentioned has to do with CC/CXX/CFLAGS/CXXFLAGS, etc. so I guess the
CC changes can be made under that bug.

However, I don't have time to test it fully at the moment.  I'm flying to the
Bay Area in about 12 hours, and I have about a week's worth of work to do before
then...  I gave it a really quick test (just building NSS), and it seem fine,
and just reading it, it looks like it's doing the right thing.

Thanks to everyone for considering these patches and all the work involved in
checking them in.

Regards,
  -Jeremy

Comment 42

[Wan-Teh Chang]

Comment on attachment 71379 [details] [diff] [review]
Patch for NSS FreeBSD.mk

Jeremy,

Thanks for reviewing my modifications to your patch.

I omittedthe following CC changes in your patch to
coreconf/FreeBSD.mk:

>-CC			= gcc
>-CCC			= g++
>+CC			?= gcc
>+CXX			?= g++
>+CCC			= ${CXX}

I originally thought the question marks were typos
or corrupted characters when you attached the patch.
But it seems that you were saying they are actually
correct?  Do they mean "set CC to gcc only if CC is
not yet defined"?

Comment 43

[Wan-Teh Chang]

Comment on attachment 71827 [details] [diff] [review]
Patch for NSS FreeBSD.mk

With Jeremy's review and testing, I checked in this
patch into the tip of NSS.  These changes will appear
in NSS_CLIENT_TAG the next time we update that tag.