Last Comment Bug 729067 - changes LDFLAGS before calling js/src/configure
: changes LDFLAGS before calling js/src/configure
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All Windows 7
: -- normal (vote)
: mozilla14
Assigned To: Jacek Caban
: Gregory Szorc [:gps]
Depends on:
Blocks: 720621
  Show dependency treegraph
Reported: 2012-02-21 03:36 PST by Jacek Caban
Modified: 2012-03-15 08:00 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

fix v1.0 (2.17 KB, patch)
2012-02-21 03:36 PST, Jacek Caban
mh+mozilla: review+
khuey: review+
Details | Diff | Splinter Review
fix v1.0 (3.15 KB, patch)
2012-03-02 02:41 PST, Jacek Caban
no flags Details | Diff | Splinter Review
alternative patch (668 bytes, patch)
2012-03-02 04:10 PST, Jacek Caban
no flags Details | Diff | Splinter Review
fix v2.1 (3.75 KB, patch)
2012-03-14 05:17 PDT, Jacek Caban
mh+mozilla: review+
Details | Diff | Splinter Review

Description Jacek Caban 2012-02-21 03:36:09 PST
Created attachment 599088 [details] [diff] [review]
fix v1.0

Bug 720621 introduced a pair of export/unset calls on LDFLAGS, which clears its variable, so configure ran like:

LDFLAGS="..." make -f build

won't work for js/src/configure. While we could just fix this one case, I propose more radical solution which makes already existing variable preserving stronger.
Comment 1 Mike Hommey [:glandium] 2012-02-21 03:52:02 PST
Comment on attachment 599088 [details] [diff] [review]
fix v1.0

I tend to agree, here, but I prefer to get some more eyes on this thing. (Ted or Kyle, whoever comes first)
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-21 06:40:25 PST
Comment on attachment 599088 [details] [diff] [review]
fix v1.0

Review of attachment 599088 [details] [diff] [review]:

Seems reasonable enough to me.
Comment 3 Jacek Caban 2012-02-21 06:48:38 PST
Thanks for reviews, pushed to m-i:
Comment 4 Ed Morley [:emorley] 2012-02-21 07:06:33 PST
Backed out for failing to build on all platforms. It may have just needed a clobber, but without a green try run I was hesitant to just clobber/retrigger, since in the meantime we could have had 10 other csets land all burning.
Comment 5 Jacek Caban 2012-02-21 07:29:03 PST
Thanks for taking care of the backout. It looks it needs a clobber, I've pushed the patch to try to make sure:
Comment 6 Jacek Caban 2012-03-02 02:41:09 PST
Created attachment 602297 [details] [diff] [review]
fix v1.0

It came out that clobbering would not be enough. libffi's configure script is invoked in a bit hackish way for MSVC, so we need to make sure CFLAGS and LDFLAGS are cleared before its invocation. The attached patch is green on try:
Comment 7 Mike Hommey [:glandium] 2012-03-02 02:44:17 PST
Comment on attachment 602297 [details] [diff] [review]
fix v1.0

Review of attachment 602297 [details] [diff] [review]:

::: js/src/
@@ +5382,5 @@
>      # Use a wrapper script for cl and ml that looks more like gcc.
>      # autotools can't quite handle an MSVC build environment yet.
>      ac_configure_args="$ac_configure_args LD=link CPP=\"cl -nologo -EP\" SHELL=sh.exe"
> +    export LDFLAGS=
> +    export CFLAGS=

In practice, what were the flags before your patch, and what are they after? I can't tell if this is harmful without knowing that.
Comment 8 Jacek Caban 2012-03-02 04:06:52 PST
They are exactly the same before and after the patch for me and contain LDFLAGS and CFLAGS that are the result of running js's configure script. These are values I get on MSVC build:

CFLAGS="-TC -nologo -W3 -Gy -Fd$(COMPILE_PDBFILE) -we4553"

The difference is that with my patch, these values get exported by top level configure, so js/src/configure modifies exported values and thus they are visible to libffi's configure.

The more I think about it, the less I like the idea of exporting these values. Perhaps avoiding the export/unset calls in the first place is the right way to go after all... I will attach an alternative patch.
Comment 9 Jacek Caban 2012-03-02 04:10:00 PST
Created attachment 602307 [details] [diff] [review]
alternative patch

The patch that avoids export/unset calls around nspr configure. Since it was introduced for Android as part of bug 720621, I can't test it locally. If we decided to go with this patch, is try build enough for testing?
Comment 10 Mike Hommey [:glandium] 2012-03-02 08:02:20 PST
Comment on attachment 602307 [details] [diff] [review]
alternative patch

Arguably, we should be doing that for all variables.

As for testing, NSPR_LDFLAGS is only set if using Android NDK >= r6b, and our build infrastructure uses r5c, so try won't tell you if that actually works.
Comment 11 Jacek Caban 2012-03-14 05:16:16 PDT
My last attempt didn't work, because nspr configure can't handle
./configure CFLAGS="..."
syntax. I think it's because of ancient autoconf-2.13 used, but I'm not sure (my experiments suggest that's the reason, it's hard to find any info about it).

Thus, I'm back to the solution from comment 6. I will attach a patch that follows the same pattern, but does not export variables on their own. So, if these variables are exported for some reason, we'd behave correctly, but don't export them unless needed.

The patch succeeded on try:
Comment 12 Jacek Caban 2012-03-14 05:17:17 PDT
Created attachment 605716 [details] [diff] [review]
fix v2.1
Comment 13 Jacek Caban 2012-03-14 07:11:03 PDT
Thanks for the review.
Comment 14 Marco Bonardo [::mak] 2012-03-15 08:00:02 PDT

Note You need to log in before you can comment on or make changes to this bug.