Last Comment Bug 681659 - Execute mozconfig-find just once and make sure mozconfig detection errors are handled properly
: Execute mozconfig-find just once and make sure mozconfig detection errors are...
Status: RESOLVED FIXED
fixed-in-bs
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: Siddharth Agarwal [:sid0] (inactive)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-24 08:42 PDT by Siddharth Agarwal [:sid0] (inactive)
Modified: 2011-10-16 21:56 PDT (History)
6 users (show)
khuey: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (5.65 KB, patch)
2011-08-24 08:42 PDT, Siddharth Agarwal [:sid0] (inactive)
ted: review+
Details | Diff | Splinter Review

Description Siddharth Agarwal [:sid0] (inactive) 2011-08-24 08:42:46 PDT
Created attachment 555418 [details] [diff] [review]
patch v1

Performing mozconfig-find multiple times can lead to different areas of our build system reading different mozconfigs. In particular client.mk might do it from the srcdir but configure might do it from the objdir. This is pretty bad :(

We don't handle mozconfig detection errors properly either -- we'll happily assign an error string to the MOZCONFIG variable.

I'm pretty sure you're going to kill me for this patch, Ted. :) This was the only portable way I could find of testing whether a variable is set (whether to an empty string or not), but a better way would definitely be welcome.
Comment 1 Ted Mielczarek [:ted.mielczarek] 2011-09-08 11:55:20 PDT
Comment on attachment 555418 [details] [diff] [review]
patch v1

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

I don't love this patch, but I don't see a cleaner way to do what you're doing. I guess I don't love our mozconfig handling in general.

::: build/autoconf/mozconfig2client-mk
@@ +102,5 @@
> +isfoundset=${FOUND_MOZCONFIG+yes}
> +if [ -z $isfoundset ]; then
> +  FOUND_MOZCONFIG=`$scriptdir/mozconfig-find $topsrcdir`
> +  if [ $? -ne 0 ]; then
> +    echo "\$(error Fix above errors before continuing.)" >> $tmp_file

nit: you can use single quotes to avoid having to escape the $
Comment 2 Siddharth Agarwal [:sid0] (inactive) 2011-09-08 13:35:30 PDT
I updated the patch to take bug 684782 into account.

https://hg.mozilla.org/projects/build-system/rev/57d1c5019805
Comment 3 Siddharth Agarwal [:sid0] (inactive) 2011-09-08 14:17:06 PDT
Checked into comm-central too. https://hg.mozilla.org/comm-central/rev/7e389dc6008e
Comment 4 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-15 19:20:15 PDT
https://hg.mozilla.org/mozilla-central/rev/57d1c5019805
Comment 5 neil@parkwaycc.co.uk 2011-09-17 08:11:16 PDT
Comment on attachment 555418 [details] [diff] [review]
patch v1

>     echo `abspath $_config`
Compare return cond ? true : false;
[didn't spot this before when you landed bug 675691]

>-if [ "$MOZCONFIG" ]
You missed a previous use of MOZCONFIG.

>+isfoundset=${FOUND_MOZCONFIG+yes}
Did you mean :+ ?
Comment 6 Siddharth Agarwal [:sid0] (inactive) 2011-09-17 09:31:25 PDT
(In reply to neil@parkwaycc.co.uk from comment #5)
> Comment on attachment 555418 [details] [diff] [review]
> patch v1
> 
> >     echo `abspath $_config`
> Compare return cond ? true : false;
> [didn't spot this before when you landed bug 675691]

I didn't understand this comment.

> >+isfoundset=${FOUND_MOZCONFIG+yes}
> Did you mean :+ ?

No, + is the right thing. I want to distinguish unset from set, not empty from non-empty. (Set but empty means we don't have a mozconfig, which is fine these days.)
Comment 7 Siddharth Agarwal [:sid0] (inactive) 2011-09-17 12:39:44 PDT
Filed bug 687275 to address the two issues Neil pointed out.
Comment 8 neil@parkwaycc.co.uk 2011-09-17 15:51:01 PDT
(In reply to Siddharth Agarwal from comment #0)
> Performing mozconfig-find multiple times can lead to different areas of our
> build system reading different mozconfigs. In particular client.mk might do
> it from the srcdir but configure might do it from the objdir. This is pretty
> bad :(
No, it's very useful! I used to just set MOZCONFIG=.mozconfig and configure would find my .mozconfig in my objdir and therefore I could have mulitple objdirs without having to change my MOZCONFIG all the time.

Now I can imagine that as soon as you find a mozconfig you might want to cache that, but until then, I think you should keep looking, hence the :+ comment.
Comment 9 Siddharth Agarwal [:sid0] (inactive) 2011-10-16 21:56:03 PDT
(In reply to neil@parkwaycc.co.uk from comment #8)
> Now I can imagine that as soon as you find a mozconfig you might want to
> cache that, but until then, I think you should keep looking, hence the :+
> comment.

That might make sense if we didn't support the lack of a mozconfig, but now that we do, I don't think we should keep looking. The problem where we kept looking was why we fixed bug 675691 in the first place.

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