Closed Bug 1255312 Opened 4 years ago Closed 4 years ago
Js subconfigure is being passed --enable-application=foo
58 bytes, text/x-review-board-request
The js subconfigure is defaulting --enable-application/--enable-project to js, but it's being fed e.g. --enable-application=mobile/android on android builds.
Review commit: https://reviewboard.mozilla.org/r/39157/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/39157/
Attachment #8728818 - Flags: review?(gps)
Comment on attachment 8728818 [details] MozReview Request: Bug 1255312 - Force --enable-application/--enable-project=js when calling js subconfigure https://reviewboard.mozilla.org/r/39157/#review36063
We need a less shortsighted fix. We already need --with-external-source-dir not to be passed down to js for c-c, and we're going to have the same kind of problem when we start moving things out of old-configure that are supported in top-level and not in js. So let's see. Options known to old-configure.ins only are all listed in build/moz.configure/old.configure. All those should be passed down if they appear on the command line. With js/moz.configure being included by toolkit/moz.configure, we've committed to options known to js/src/configure being known to top-level configure, which is good. So we'd definitely want to pass down any of those options appearing in js/moz.configure. And we don't want to pass down any other option.
The complication is that passing down options to js/src/configure is done by old-configure...
... but... options handled by python configure for the top-level don't need to be passed to top-level old-configure.in, since it doesn't handle those options anyways...
Attachment #8728818 - Attachment is obsolete: true
So far, we've been passing down all configure_args from mozconfig as well as every flag appearing on sys.argv. This is overly broad and causes problems for some options, like --enable-application. However, we don't need all these options to be passed. For the top-level old-configure, we need to pass the flags it can handle, as well as the flags that we want passed down to js/src/configure. For js/src/old-configure, we only need to pass the flags it can handle. The flags an old-configure can handle is defined by the list of flags in @old_configure_options. The list of flags to pass down to js/src/configure is defined by extra_old_configure_args. And since the mozconfig configure_args are being injected into python configure processing, the list of values we get in old_configure includes the mozconfig configure_args. Review commit: https://reviewboard.mozilla.org/r/39405/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/39405/
Comment on attachment 8729355 [details] MozReview Request: Bug 1255312 - Limit the options we pass down to old-configure https://reviewboard.mozilla.org/r/39405/#review36247 ::: build/moz.configure/old.configure:377 (Diff revision 1) > + # We also pass it the options from js/moz.configure so that it can pass > + # them down to js/src/configure. Note this list is empty when running > + # js/src/configure, in which case we don't need to pass those options > + # to old-configure since old-configure doesn't handle them anyways. > + if extra_old_configure_args: > + cmd += extra_old_configure_args If I set "--enable-js-shell" in my mozconfig, commenting out this loop doesn't have any interesting effect on the resulting config.status, because we're including js/moz.configure in toolkit/moz.configure. Is this necessary as long as that's the case?
--enable-js-shell is special, because its default in top-level is disabled and in js/src it is enabled. So if it's not passed down properly, you get the default from js/src, which is enabled, so you don't see that, in fact, you broke it by removing the extra_old_configure_args. If you change the default in js/moz.configure, you should see a difference.
Ok, thanks for the explanation.
Comment on attachment 8729355 [details] MozReview Request: Bug 1255312 - Limit the options we pass down to old-configure https://reviewboard.mozilla.org/r/39405/#review36255
Attachment #8729355 - Flags: review?(cmanchester) → review+
You need to log in before you can comment on or make changes to this bug.