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

RESOLVED FIXED in mozilla9

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sid0, Assigned: sid0)

Tracking

unspecified
mozilla9
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-bs)

Attachments

(1 attachment)

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.
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+
(Assignee)

Comment 2

6 years ago
I updated the patch to take bug 684782 into account.

https://hg.mozilla.org/projects/build-system/rev/57d1c5019805
Whiteboard: fixed-in-bs
(Assignee)

Comment 3

6 years ago
Checked into comm-central too. https://hg.mozilla.org/comm-central/rev/7e389dc6008e
https://hg.mozilla.org/mozilla-central/rev/57d1c5019805
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla9

Comment 5

6 years ago
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 :+ ?
(Assignee)

Comment 6

6 years ago
(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.)
(Assignee)

Comment 7

6 years ago
Filed bug 687275 to address the two issues Neil pointed out.

Comment 8

6 years ago
(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.
(Assignee)

Comment 9

6 years ago
(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.
You need to log in before you can comment on or make changes to this bug.