Last Comment Bug 675691 - Get rid of all the mozconfig guesswork
: Get rid of all the mozconfig guesswork
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Siddharth Agarwal [:sid0] (inactive)
:
Mentors:
Depends on:
Blocks: 678475 682897 684782
  Show dependency treegraph
 
Reported: 2011-08-01 11:27 PDT by Siddharth Agarwal [:sid0] (inactive)
Modified: 2011-12-05 11:21 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (1.27 KB, patch)
2011-08-10 11:59 PDT, Siddharth Agarwal [:sid0] (inactive)
ted: review-
bugspam.Callek: feedback-
Details | Diff | Splinter Review
patch with erroring out (2.16 KB, patch)
2011-08-10 12:27 PDT, Siddharth Agarwal [:sid0] (inactive)
bugspam.Callek: feedback+
Details | Diff | Splinter Review
patch v2 with erroring out (1.95 KB, patch)
2011-08-11 05:32 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details | Diff | Splinter Review
patch v2 with erroring out, rebased (1.99 KB, patch)
2011-08-15 15:21 PDT, Siddharth Agarwal [:sid0] (inactive)
ted: review+
Details | Diff | Splinter Review

Description Siddharth Agarwal [:sid0] (inactive) 2011-08-01 11:27:33 PDT
One of the principles our build system follows is to make things as explicit as possible and have very little magic in it. Our mozconfig-guessing code violates this principle, and since bad things can happen if the wrong mozconfig's picked up, this seems rather serious.

My suggestion would be to get rid of all the guesswork entirely. Make things simple.

- No mozconfig by default (this already works.)
- Maybe pick up and use $topsrcdir/.mozconfig. I'd prefer that any mozconfig use be completely explicit, but we might want to have this as a transitional measure.
- For anything else, whatever $MOZCONFIG is, and if a relative path relative to the current directory. That includes backing things like bug 341223 out.
Comment 1 Siddharth Agarwal [:sid0] (inactive) 2011-08-10 11:59:13 PDT
Created attachment 552156 [details] [diff] [review]
patch v1

All right, this does what I said.
Comment 2 Justin Wood (:Callek) 2011-08-10 12:05:43 PDT
Comment on attachment 552156 [details] [diff] [review]
patch v1

as a nit, can we go at least one cycle with an error or warning if the other forms exist/are used [when no other format is found]?

Especially since a mozconfig is no longer a hard requirement anymore.

That is *IF* we do this.
Comment 3 Siddharth Agarwal [:sid0] (inactive) 2011-08-10 12:06:41 PDT
That definitely sounds like a good idea.
Comment 4 Siddharth Agarwal [:sid0] (inactive) 2011-08-10 12:27:47 PDT
Created attachment 552174 [details] [diff] [review]
patch with erroring out

This errors out whenever we'd have picked up a different implicit mozconfig earlier.
Comment 5 Justin Wood (:Callek) 2011-08-10 12:39:49 PDT
Comment on attachment 552174 [details] [diff] [review]
patch with erroring out


>+# We used to support a number of other implicit .mozconfig locations. We now
>+# detect if we were about to use any of these locations and issue an error if we
>+# find any.
>+for _config in "$topsrcdir/mozconfig" \
>+               "$topsrcdir/mozconfig.sh" \
>+               "$topsrcdir/myconfig.sh" \
>+               "$HOME/.mozconfig" \
>+               "$HOME/.mozconfig.sh" \
>+               "$HOME/.mozmyconfig.sh"
>+               "$topsrcdir/.mozconfig"

Not this last line. (whops)

>+do
>+  if test -f "$_config"; then
>+    echo "You currently have a mozconfig at \"$_config\". This implicit location is no longer supported. Please move it to $topsrcdir/.mozconfig or specify it explicitly.";

Nit "...explicitly via \$MOZCONFIG."
Comment 6 Siddharth Agarwal [:sid0] (inactive) 2011-08-10 12:42:04 PDT
Oops, that's what I get for not hitting save before creating the patch.
Comment 7 Siddharth Agarwal [:sid0] (inactive) 2011-08-11 05:32:42 PDT
Created attachment 552343 [details] [diff] [review]
patch v2 with erroring out

This fixes the issues Callek mentioned.
Comment 8 Siddharth Agarwal [:sid0] (inactive) 2011-08-15 15:21:50 PDT
Created attachment 553282 [details] [diff] [review]
patch v2 with erroring out, rebased

Bug 678475 landed first and bitrotted this.
Comment 9 Ted Mielczarek [:ted.mielczarek] 2011-08-23 12:39:37 PDT
Comment on attachment 552156 [details] [diff] [review]
patch v1

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

I think I'd prefer the other option for now.
Comment 10 Ted Mielczarek [:ted.mielczarek] 2011-08-23 12:40:15 PDT
Comment on attachment 553282 [details] [diff] [review]
patch v2 with erroring out, rebased

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

This looks reasonable.
Comment 11 Siddharth Agarwal [:sid0] (inactive) 2011-08-24 00:31:38 PDT
A couple of changes didn't make it through the rebase, so I added them in.

https://hg.mozilla.org/projects/build-system/rev/c3657f68bd66
Comment 12 Mike Hommey [:glandium] 2011-08-25 01:52:02 PDT
http://hg.mozilla.org/mozilla-central/rev/c3657f68bd66
Comment 13 Mounir Lamouri (:mounir) 2011-08-26 03:01:35 PDT
What's the win with this patch? Is it a huge burden to maintain the few lines of code that tries to find where the mozconfig is?
I've the feeling we are regressing with that patch: it is going to make some developers life harder and [at a first glance] it doesn't seem that some developers life is going to be easier...

As a side note, when a software tells me "we found you want to use this file in this directory but really, we don't want to use it anymore even if we were using it before so just rename the file to make it hidden", I tends to think something is wrong...
Comment 14 Siddharth Agarwal [:sid0] (inactive) 2011-08-26 04:45:21 PDT
(In reply to Mounir Lamouri (:volkmar) from comment #13)
> What's the win with this patch? Is it a huge burden to maintain the few
> lines of code that tries to find where the mozconfig is?

No, but that's not the point of this patch. As I mentioned in comment 0, build system policy is to minimize magic and ensure that any non-default configurations are explicitly opted into by the user. For instance, on Windows if the SDK is too old we error out and ask the user to explicitly disable features available in newer SDKs. We could have just disabled those features ourselves, but doing so without the developer actually knowing is a really bad idea, I think. Ted knows more about this, but I'm pretty sure that the reason build system has this policy was that for a while we shipped Linux builds with a feature disabled, simply because tinderboxes didn't have the headers for that feature installed.

This patch is along the same lines. The default configuration is no mozconfig at all, and any non-default configurations have to be opted into by the user by setting the environment variable.

> As a side note, when a software tells me "we found you want to use this file
> in this directory but really, we don't want to use it anymore even if we
> were using it before so just rename the file to make it hidden", I tends to
> think something is wrong...

Note that we error out if and only if we would have used that implicit location. So if the MOZCONFIG environment variable is already specified, the presence of $HOME/.mozconfig won't cause the build to error out.
Comment 15 Mounir Lamouri (:mounir) 2011-08-26 05:31:08 PDT
(In reply to Siddharth Agarwal [:sid0] from comment #14)
> (In reply to Mounir Lamouri (:volkmar) from comment #13)
> > What's the win with this patch? Is it a huge burden to maintain the few
> > lines of code that tries to find where the mozconfig is?
> 
> No, but that's not the point of this patch. As I mentioned in comment 0,
> build system policy is to minimize magic and ensure that any non-default
> configurations are explicitly opted into by the user. For instance, on
> Windows if the SDK is too old we error out and ask the user to explicitly
> disable features available in newer SDKs. We could have just disabled those
> features ourselves, but doing so without the developer actually knowing is a
> really bad idea, I think. Ted knows more about this, but I'm pretty sure
> that the reason build system has this policy was that for a while we shipped
> Linux builds with a feature disabled, simply because tinderboxes didn't have
> the headers for that feature installed.
> 
> This patch is along the same lines. The default configuration is no
> mozconfig at all, and any non-default configurations have to be opted into
> by the user by setting the environment variable.

I do not see any similarity with showing an error when some default-enabled features are not compatible with the current SDK and showing an error when the mozconfig file is the root directory but isn't hidden.
In the first case, you do not want the user to compile something and have a build failure and you do not want to have something magically disabled while she/he might think it's enabled because it's enabled by default and wasn't disabled explicitly. I definitely agree that auto-magic configure flags are wrong.
However, the second case is about something that is pretty obvious the user *want* but the build system doesn't allow. Indeed, using $SRCDIR/mozconfig as a mozconfig might be some kind of auto-magic too but that's the kind of auto-magic the user might want. Instead of increasing the user experience, not doing so is decreasing it.
As a side note, it would seem correct to show an error if $SRCDIR/mozconfig AND $SRCDIR/.mozconfig are present because here, it's not obvious what the user want. If $SRCDIR/mozconfig and $HOME/mozconfig are present, likely the user wants to use the local file. That's what gdb does for example.

> > As a side note, when a software tells me "we found you want to use this file
> > in this directory but really, we don't want to use it anymore even if we
> > were using it before so just rename the file to make it hidden", I tends to
> > think something is wrong...
> 
> Note that we error out if and only if we would have used that implicit
> location. So if the MOZCONFIG environment variable is already specified, the
> presence of $HOME/.mozconfig won't cause the build to error out.

Using an environment variable is a pain in general. I have about half a dozen mozilla-central trees, some of them with different mozconfig. Should I set MOZCONFIG env variable every time I open a new shell to work on one of these trees?
Comment 16 Siddharth Agarwal [:sid0] (inactive) 2011-08-26 08:20:01 PDT
(In reply to Mounir Lamouri (:volkmar) from comment #15)
> I do not see any similarity with showing an error when some default-enabled
> features are not compatible with the current SDK and showing an error when
> the mozconfig file is the root directory but isn't hidden.

Hidden files being indicated by an initial dot is a UNIXism and is not universal. From the build system's perspective, .mozconfig and mozconfig are different names and we have to pick one. I picked .mozconfig because it was the first implicit location we looked at.

> However, the second case is about something that is pretty obvious the user
> *want* but the build system doesn't allow. Indeed, using $SRCDIR/mozconfig
> as a mozconfig might be some kind of auto-magic too but that's the kind of
> auto-magic the user might want.

But that's why I left $SRCDIR/.mozconfig in.

> Instead of increasing the user experience,
> not doing so is decreasing it.
> As a side note, it would seem correct to show an error if $SRCDIR/mozconfig
> AND $SRCDIR/.mozconfig are present because here, it's not obvious what the
> user want.

That would come at the cost of a fair bit of additional complexity. What benefit would it serve that the simple operation of asking people to rename their mozconfigs wouldn't?

> If $SRCDIR/mozconfig and $HOME/mozconfig are present, likely the
> user wants to use the local file. That's what gdb does for example.

It would make sense only if we combined the mozconfigs, sort of like Mercurial looks at .hg/hgrc then at ~/.hgrc for settings. We don't support that right now, and given how small mozconfigs are, I'm not convinced of the value of supporting it.

> 
> Using an environment variable is a pain in general. I have about half a
> dozen mozilla-central trees, some of them with different mozconfig. Should I
> set MOZCONFIG env variable every time I open a new shell to work on one of
> these trees?

How were you doing it earlier, and how does my patch actually change things for you? The only difference for you should be the trivial one of having to rename your mozconfigs.
Comment 17 Mike Hommey [:glandium] 2011-08-26 08:37:24 PDT
(In reply to Siddharth Agarwal [:sid0] from comment #16)
> It would make sense only if we combined the mozconfigs, sort of like
> Mercurial looks at .hg/hgrc then at ~/.hgrc for settings. We don't support
> that right now, and given how small mozconfigs are, I'm not convinced of the
> value of supporting it.

I can certainly see value in supporting it (although I probably wouldn't use it): set global options like compiler location, always build debug, always disable such feature because you don't have the necessary SDK, and then have local configuration for a given working copy.
Comment 18 Nickolay_Ponomarev 2011-09-08 12:06:35 PDT
https://developer.mozilla.org/En/Developer_Guide/Build_Instructions/Mac_OS_X_Prerequisites mentions $HOME/.mozconfig in step 6 of the quick start and should be updated.
Comment 19 Eric Shepherd [:sheppy] 2011-12-02 13:38:50 PST
Anyone intimately familiar with this that would like to summarize the change to make updating docs easier?

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