Closed Bug 975011 Opened 10 years ago Closed 10 years ago

configure, build nsprpub for js/src standalone builds

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla33

People

(Reporter: sfink, Assigned: sfink)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 8 obsolete files)

6.07 KB, text/plain
Details
25.65 KB, patch
glandium
: review+
Details | Diff | Splinter Review
28.79 KB, patch
Details | Diff | Splinter Review
I have the 1.9.0pre MozillaBuild installed.

When I try to build nspr via configure; make, it fails when it runs make export from within objdir/nspr/pr/include/md.

The problem is that when mozmake or pymake run the command

  nsinstall -m 444 .../_win95.cfg objdir/dist/include/nspr

it silently (?!) fails. Here's the actual log output:

nsinstall -m 444 ../../../../.././nsprpub/pr/include/md/_win95.cfg /c/Users/sfink/src/MI-GC/objdir/dist/include/nspr
mv -f /c/Users/sfink/src/MI-GC/objdir/dist/include/nspr/_win95.cfg /c/Users/sfink/src/MI-GC/objdir/dist/include/nspr/prcpucfg.h
mv: cannot stat `/c/Users/sfink/src/MI-GC/objdir/dist/include/nspr/_win95.cfg': No such file or directory

I am suspecting something with LF vs CRLF, because if I change the Makefile

-        $(INSTALL) -m 444 $(srcdir)/$(MDCPUCFG_H) $(dist_includedir)
+        sh -c '$(INSTALL) -m 444 $(srcdir)/$(MDCPUCFG_H) $(dist_includedir)'

then it works. So does

-        $(INSTALL) -m 444 $(srcdir)/$(MDCPUCFG_H) $(dist_includedir)
+        $(INSTALL) -m 444 $(srcdir)/$(MDCPUCFG_H) $(dist_includedir) #

(adding a comment character at the end.)

Also, the log of the full script output starts out with LF line endings, but switches to CRLF as soon as it does the make for nspr. Though that doesn't really prove anything.
Attached file Build log
Blocks: 972089
Attached patch imported patch nspr-config (obsolete) — Splinter Review
glandium recommended calling NSPR configure + build from js/src. This patch works well enough to get me a Windows build. I haven't checked it out anywhere else yet, but here's a try push: https://tbpl.mozilla.org/?tree=Try&rev=0761c986dade
Attachment #8379379 - Flags: review?(mh+mozilla)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment on attachment 8379379 [details] [diff] [review]
imported patch nspr-config

Doing too much tweaking to get this to work, so cancelling review for now.
Attachment #8379379 - Flags: review?(mh+mozilla)
Attached patch patch (obsolete) — Splinter Review
Ok, got the darn thing working. Note that there's some strange order dependence between, I think, the libffi subconfigure and the nspr configure on b2g builds. (Specifically, it seems like there's a -std=c99 that needs to be set in order for nspr to find pthread.h.)

https://tbpl.mozilla.org/?tree=Try&rev=7af560a044ee
Attachment #8381820 - Flags: review?(mh+mozilla)
Attachment #8379379 - Attachment is obsolete: true
Summary: nsinstall fails when run by mozmake or pymake, fine if run by msys make → configure, build nsprpub for js/src standalone builds
Comment on attachment 8381820 [details] [diff] [review]
patch

Review of attachment 8381820 [details] [diff] [review]:
-----------------------------------------------------------------

::: aclocal.m4
@@ +10,5 @@
>  builtin(include, build/autoconf/toolchain.m4)dnl
>  builtin(include, build/autoconf/ccache.m4)dnl
>  builtin(include, build/autoconf/wrapper.m4)dnl
>  builtin(include, build/autoconf/nspr.m4)dnl
> +builtin(include, build/autoconf/nsprpub.m4)dnl

make that build-nspr.m4 or nspr-build.m4

::: build/autoconf/nsprpub.m4
@@ +29,5 @@
> +                          See --with-nspr-cflags for more details.],
> +    NSPR_LIBS=$withval)
> +
> +MOZ_ARG_ENABLE_BOOL(nsprpub,
> +[  --enable-nsprpub        Configure and build NSPR from source tree],

--enable-nspr-build? --enable-nspr?

@@ +31,5 @@
> +
> +MOZ_ARG_ENABLE_BOOL(nsprpub,
> +[  --enable-nsprpub        Configure and build NSPR from source tree],
> +    MOZ_USE_NSPRPUB=1,
> +    MOZ_USE_NSPRPUB=)

I'd rather not expose this from top-level configure. Please wrap with something like:
ifelse([$1],:,MOZ_ARG_ENABLE_BOOL(...))

and invoke MOZ_CONFIG_NSPR with a dummy argument in js/src/configure.in. (and double check that --enable-nsprpub doesn't appear in configure --help for top-level but does appear for js/src/configure --help.

@@ +37,5 @@
> +if test -z "$BUILDING_JS" || test -n "$JS_STANDALONE"; then
> +  _IS_OUTER_CONFIGURE=1
> +fi
> +
> +if test -n "$_IS_OUTER_CONFIGURE"; then

This test unfortunately doesn't do what you think it does. MOZ_ARG_WITH_BOOL is at the m4 level, and part of it expands to a completely unrelated place, unwrapped by that if. You need the m4 ifelse like above, in which case you need _IS_OUTER_CONFIGURE defined at the m4 level, which ends up being complicated. The same $1 trick as above would do it, but i'm not sure why you want --with-system-nspr in one configure and not the other.

@@ +49,5 @@
> +    _USE_SYSTEM_NSPR=1 )
> +fi
> +
> +JS_POSIX_NSPR=unset
> +if test -n "$JS_STANDALONE"; then

That should be dependent on BUILDING_JS, but you hit the same issue as above, so ifelse([$1],:,...) again

@@ +87,5 @@
> +    nspr_opts="x$nspr_opts"
> +    which_nspr="source-tree"
> +fi
> +if test "$JS_POSIX_NSPR" = unset; then
> +    : "can we use negation yet?"

yes

@@ +101,5 @@
> +
> +if test -z "$nspr_opts"; then
> +  if test -z "$BUILDING_JS"; then
> +    dnl Toplevel configure defaults to using nsprpub from the source tree
> +    MOZ_USE_NSPRPUB=1

MOZ_BUILD_NSPR

@@ +107,5 @@
> +    dnl JS configure defaults to emulated NSPR if available, falling back to nsprpub
> +    JS_POSIX_NSPR="$JS_POSIX_NSPR_DEFAULT"
> +    if test -z "$JS_POSIX_NSPR"; then
> +      MOZ_USE_NSPRPUB=1
> +    fi

That's what we can remove when all automation switches to building nspr through the build system, right?

@@ +130,5 @@
> +if test -n "$_USE_SYSTEM_NSPR" -o -n "$NSPR_CFLAGS" -o -n "$NSPR_LIBS"; then
> +    AM_PATH_NSPR($NSPR_MINVER, [MOZ_NATIVE_NSPR=1], [AC_MSG_ERROR([you do not have NSPR installed or your version is older than $NSPR_MINVER.])])
> +fi
> +
> +if test -n "$MOZ_NATIVE_NSPR" && test -z "$JS_POSIX_NSPR"; then

You can't have both MOZ_NATIVE_NSPR and JS_POSIX_NSPR at the same time per the mutual exclusion test above.

@@ +137,5 @@
> +    AC_TRY_COMPILE([#include "prtypes.h"],
> +                [#ifndef PR_STATIC_ASSERT
> +                 #error PR_STATIC_ASSERT not defined or requires including prtypes.h
> +                 #endif],
> +                [MOZ_NATIVE_NSPR=1],

I know this is how it is currently, but you might as well remove this useless assignment, since MOZ_NATIVE_NSPR is already 1 when you reach here.
Attachment #8381820 - Flags: review?(mh+mozilla) → feedback+
Attached patch patchSplinter Review
(In reply to Mike Hommey [:glandium] from comment #6)
> @@ +37,5 @@
> > +if test -z "$BUILDING_JS" || test -n "$JS_STANDALONE"; then
> > +  _IS_OUTER_CONFIGURE=1
> > +fi
> > +
> > +if test -n "$_IS_OUTER_CONFIGURE"; then
> 
> This test unfortunately doesn't do what you think it does. MOZ_ARG_WITH_BOOL
> is at the m4 level, and part of it expands to a completely unrelated place,
> unwrapped by that if. You need the m4 ifelse like above, in which case you
> need _IS_OUTER_CONFIGURE defined at the m4 level, which ends up being
> complicated.

Y'know, this bothered me when I wrote it, and I remember thinking "how could this possibly work?" but then forgot about it when my tests passed.

> The same $1 trick as above would do it, but i'm not sure why
> you want --with-system-nspr in one configure and not the other.

Er... yeah, that doesn't make sense. It wasn't intentional.

> @@ +107,5 @@
> > +    dnl JS configure defaults to emulated NSPR if available, falling back to nsprpub
> > +    JS_POSIX_NSPR="$JS_POSIX_NSPR_DEFAULT"
> > +    if test -z "$JS_POSIX_NSPR"; then
> > +      MOZ_USE_NSPRPUB=1
> > +    fi
> 
> That's what we can remove when all automation switches to building nspr
> through the build system, right?

No. JS is working on removing the use of NSPR entirely. The POSIX "emulation" layer is already the default on linux and osx for standalone JS builds. A contributor has a non-POSIX replacement for NSPR on Windows that more or less works, but hasn't landed. I don't know what the plans are when building JS as part of Gecko; I think for the time being the intent is to continue using NSPR.

When all automation switches to building nspr through the build system, we could maybe remove --with-nspr-libs and --with-nspr-cflags?

> @@ +87,5 @@
> > +    nspr_opts="x$nspr_opts"
> > +    which_nspr="source-tree"
> > +fi
> > +if test "$JS_POSIX_NSPR" = unset; then
> > +    : "can we use negation yet?"
> 
> yes

If negation is allowed, what about ${var+x} ? I could maybe simplify the mutual exclusion stuff with nspr_opts="${_USE_SYSTEM_NSPR+x}${MOZ_BUILD_NSPR+x}...". Though I guess the which_nspr thing is kind of handy, so I probably wouldn't anyway.
Attachment #8385709 - Flags: review?(mh+mozilla)
Attachment #8381820 - Attachment is obsolete: true
Comment on attachment 8385709 [details] [diff] [review]
patch

Review of attachment 8385709 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/nspr-build.m4
@@ +1,5 @@
> +dnl This Source Code Form is subject to the terms of the Mozilla Public
> +dnl License, v. 2.0. If a copy of the MPL was not distributed with this
> +dnl file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +ifelse([$1],,define(CONFIGURING_JS,yes))

This is outside any AC_DEFUN, so this is not going to work properly :)

@@ +59,5 @@
> +        JS_POSIX_NSPR_DEFAULT=1
> +      fi
> +      ;;
> +  esac
> +fi

this block can probably move under ifdef([CONFIGURING_JS]

@@ +62,5 @@
> +  esac
> +fi
> +
> +ifdef([CONFIGURING_JS],[
> +MOZ_ARG_ENABLE_BOOL(posix-nspr-emulation,

Please indent
Attachment #8385709 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #8)
> Comment on attachment 8385709 [details] [diff] [review]
> patch
> 
> Review of attachment 8385709 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/autoconf/nspr-build.m4
> @@ +1,5 @@
> > +dnl This Source Code Form is subject to the terms of the Mozilla Public
> > +dnl License, v. 2.0. If a copy of the MPL was not distributed with this
> > +dnl file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +ifelse([$1],,define(CONFIGURING_JS,yes))
> 
> This is outside any AC_DEFUN, so this is not going to work properly :)

Ah, but that bug is countered by me using the wrong number of commas -- that line compares $1 to the empty string, and does the define if they're equal when I meant unequal.

Except *that* bug is countered by the lack of quoting around the define, so it happens unconditionally anyway.

:-)

Dammit, I did test this. But then I totally changed it. And didn't re-test. (I was trying to mess with the configure.in files to define some m4 macros appropriately so I could avoid the MOZ_CONFIG_NSPR(hack) thing.)
This is excellent, worth sending a message to the internals list about this flag I think.
https://hg.mozilla.org/mozilla-central/rev/7cff27cb2845
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 981162
Depends on: 981587
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff5ca7959511
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backout made it to mozilla-central:

https://hg.mozilla.org/mozilla-central/rev/ff5ca7959511

I've added an ignore range for this (from rev 7cff27cb2845 to rev ff5ca7959511) in the fuzzing harness, as well as restoring the original compilation scripts.
Uploading current version of patch. It's definitely not right yet -- see https://tbpl.mozilla.org/?tree=Try&rev=17adcc46d3f7

But I think it's a lot closer than it was. I will request testing when I at least get try green again.
Rebased, since soubok wants to try it.
Attachment #8404995 - Attachment is obsolete: true
Ok, this one fixes the b2g problem. I know of no further problems, so please give this one a spin.
Attachment #8405718 - Flags: review?(mh+mozilla)
Attachment #8405718 - Flags: feedback?(gary)
Attachment #8405718 - Flags: feedback?(choller)
Attachment #8405485 - Attachment is obsolete: true
Comment on attachment 8405718 [details] [diff] [review]
allow js/src/configure to invoke the in-tree NSPR configure

Compiling with --enable-threadsafe fails on Windows if the objdir is not in the repository. Log coming up.

(backslashes/forward slashes in the command didn't matter)
Attachment #8405718 - Flags: feedback?(gary) → feedback-
Attached file compilation error (obsolete) —
Steps:

1. Run `sh autoconf-2.13` in js/src.
2. Make a new objdir, then cd into it.
3. Run the configure command at the top of the log.
4. Run `mozmake -j1`.

I test with the objdir in various positions, either in the repository under js/src or on ~/Desktop.
You're invoking the configure script with backslashes in the path. Does it work if you use forward slashes?
Flags: needinfo?(gary)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #19)
> (backslashes/forward slashes in the command didn't matter)

Nope, as I mentioned in comment 19, I also tried the "other" slash and it didn't work either.
Flags: needinfo?(gary) → needinfo?(sphink)
Comment on attachment 8405718 [details] [diff] [review]
allow js/src/configure to invoke the in-tree NSPR configure

I tried building with --enable-threadsafe as well as --disable-threadsafe and both seemed to be working with 32 and 64 bit builds :)
Attachment #8405718 - Flags: feedback?(choller) → feedback+
Comment on attachment 8405718 [details] [diff] [review]
allow js/src/configure to invoke the in-tree NSPR configure

Review of attachment 8405718 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/nspr-build.m4
@@ +1,5 @@
> +dnl This Source Code Form is subject to the terms of the Mozilla Public
> +dnl License, v. 2.0. If a copy of the MPL was not distributed with this
> +dnl file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +ifelse([$1],,define(CONFIGURING_JS,yes))

This is still outside any AC_DEFUN. That can't work.

@@ +63,5 @@
> +        JS_POSIX_NSPR_DEFAULT=1
> +      fi
> +      ;;
> +  esac
> +fi

this block can still move under ifdef([CONFIGURING_JS]

@@ +66,5 @@
> +  esac
> +fi
> +
> +ifdef([CONFIGURING_JS],[
> +MOZ_ARG_ENABLE_BOOL(posix-nspr-emulation,

Please indent.

@@ +138,5 @@
> +  fi
> +  AC_SUBST(JS_POSIX_NSPR)
> +fi
> +
> +# A (sub)configure invoked by the toplevel configure will always receive either

there's no "or" to that "either".
Attachment #8405718 - Flags: review?(mh+mozilla) → feedback+
Incorporate glandium's suggestions. Haven't gone back to try to figure out gkw's problem yet.
Attachment #8405718 - Attachment is obsolete: true
(In reply to Steve Fink [:sfink] from comment #25)
> Created attachment 8440873 [details] [diff] [review]
> allow js/src/configure to invoke the in-tree NSPR configure
> 
> Incorporate glandium's suggestions. Haven't gone back to try to figure out
> gkw's problem yet.

works for me with the following configuration:
--enable-debug --disable-optimize --enable-gczeal --enable-threadsafe --disable-gcgenerational --enable-exact-rooting --without-intl-api --enable-ctypes --with-windows-version=601 --disable-static --disable-profile-guided-optimization --disable-vista-sdk-requirements --disable-tests
Comment on attachment 8440873 [details] [diff] [review]
allow js/src/configure to invoke the in-tree NSPR configure

This patch seems to work fine with my setup. \o/
Attachment #8440873 - Flags: feedback+
Comment on attachment 8440873 [details] [diff] [review]
allow js/src/configure to invoke the in-tree NSPR configure

Review of attachment 8440873 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, this works for Gary, and works on try: https://tbpl.mozilla.org/?tree=Try&rev=80f1dfe9f706
Attachment #8440873 - Flags: review?(mh+mozilla)
Flags: needinfo?(sphink)
Attachment #8440873 - Flags: review?(mh+mozilla) → review+
Helped to land this since there was a green try run:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d86b9442f0b

(additionally, landing early for me to verify that this works properly on my side again)
Target Milestone: mozilla30 → mozilla33
https://hg.mozilla.org/mozilla-central/rev/9d86b9442f0b
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Comment on attachment 8440873 [details] [diff] [review]
allow js/src/configure to invoke the in-tree NSPR configure

Review of attachment 8440873 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +9140,3 @@
>          if test -n "$MOZ_NO_DEBUG_RTL"; then
>              ac_configure_args="$ac_configure_args --disable-debug-rtl"
>          fi

Gah, I should I seen this. This needs to move.
Depends on: 1027149
Backed out for causing bug 1027149.
https://hg.mozilla.org/mozilla-central/rev/5a29f18ca2a0
Target Milestone: mozilla33 → ---
Status: RESOLVED → REOPENED
Flags: needinfo?(sphink)
Resolution: FIXED → ---
Ok. This was backed out a month ago and is blocking work by Gary Kwong. Who can we get to work on this in order to get this checked in again. Needinfo? has been set for a month now.
Flags: needinfo?(jdemooij)
This is the patch that landed last time, rebased on top of what I'm about to land, and with comment 31 addressed. Please test it.
Attachment #8440873 - Attachment is obsolete: true
Assignee: sphink → mh+mozilla
Status: REOPENED → ASSIGNED
Assignee: mh+mozilla → sphink
Comment on attachment 8457677 [details] [diff] [review]
Allow js/src/configure to invoke the in-tree NSPR configure

fwiw, I tested this on top of m-i rev 63c52b7ddc28 and it could still work with the fuzzing harness. feedback+
Attachment #8457677 - Flags: feedback+
Attachment #8457677 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=e262e1a6f810 is a try push of the latest version. I also checked out the arewefastyet repo, since I know it broke last time we pushed this, and it looks like now it is prepared for this patch. (I still don't know why it broke in the first place, and h4writer's on PTO.)

I'm paranoid, so I'll wait for the try build to complete before pushing this.
Flags: needinfo?(sphink)
Flags: needinfo?(jdemooij)
The try build still isn't totally done, but I gave it 15 hours, so:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a459b02a9ca4
https://hg.mozilla.org/mozilla-central/rev/a459b02a9ca4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Product: Core → Firefox Build System
Depends on: 1509757
You need to log in before you can comment on or make changes to this bug.