Closed Bug 681659 Opened 8 years ago Closed 8 years ago

Execute mozconfig-find just once and make sure mozconfig detection errors are handled properly

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla9

People

(Reporter: rain1, Assigned: rain1)

Details

(Whiteboard: fixed-in-bs)

Attachments

(1 file)

Attached patch patch v1Splinter Review
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.
Attachment #555418 - Flags: review?(ted.mielczarek)
Assignee: nobody → sagarwal
Status: NEW → ASSIGNED
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 $
Attachment #555418 - Flags: review?(ted.mielczarek) → review+
I updated the patch to take bug 684782 into account.

https://hg.mozilla.org/projects/build-system/rev/57d1c5019805
Whiteboard: fixed-in-bs
Checked into comm-central too. https://hg.mozilla.org/comm-central/rev/7e389dc6008e
https://hg.mozilla.org/mozilla-central/rev/57d1c5019805
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
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 :+ ?
(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.)
Filed bug 687275 to address the two issues Neil pointed out.
(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.
(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.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.