Move enable-release / MOZILLA_OFFICIAL support into the python configure

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Build Config
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: sylvestre, Assigned: glandium)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 wontfix, firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

Comment hidden (empty)
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
mozreview-review
Comment on attachment 8855846 [details]
Bug 1354581 - Move enable-release / MOZILLA_OFFICIAL support into the python configure

https://reviewboard.mozilla.org/r/127726/#review131096

::: build/moz.configure/toolchain.configure:976
(Diff revision 1)
> +          help='Build with more conservative, release engineering-oriented options. This may slow down builds.')
> +
> +@depends("--enable-release")
> +@imports('os')
> +def enable_release(target):
> +    if 'MOZILLA_OFFICIAL' in os.environ or target:

You should just make MOZILLA_OFFICIAL a js_option()

::: build/moz.configure/toolchain.configure:976
(Diff revision 1)
> +          help='Build with more conservative, release engineering-oriented options. This may slow down builds.')
> +
> +@depends("--enable-release")
> +@imports('os')
> +def enable_release(target):
> +    if 'MOZILLA_OFFICIAL' in os.environ or target:

if foo:
    return True
return False

can be written as:

return bool(foo)

::: build/moz.configure/toolchain.configure:981
(Diff revision 1)
> +    if 'MOZILLA_OFFICIAL' in os.environ or target:
> +        # Official release, disable the developer mode
> +        return True
> +    return False
> +
> +set_config('DEVELOPER_OPTIONS', not enable_release)

not enable_release is not going to work, you need a @depends function that does the `not`.
Attachment #8855846 - Flags: review?(mh+mozilla)
Comment hidden (mozreview-request)
(Reporter)

Comment 4

a year ago
Mike, I did the changes that you requested but I don't know how to fix this issue:
 0:02.43 checking for xargs... /usr/bin/xargs
 0:02.43 checking for rpmbuild... not found
 0:02.43 checking for autoconf... /usr/bin/autoconf2.13
 0:02.45 configure: warning: MOZILLA_OFFICIAL=: invalid host type
 0:02.51 loading cache ./config.cache
 0:02.51 configure: error: can only configure for one host and one target at a time
 0:02.51 DEBUG: This file contains any messages produced by compilers while
 0:02.51 DEBUG: running configure, to aid debugging if configure makes a mistake.
 0:02.51 DEBUG:
 0:02.51 DEBUG: configure: error: can only configure for one host and one target at a time

DEBUG: Running /bin/sh /home/sylvestre/dev/mozilla/mozilla-inbound.hg/old-configure --host=x86_64-pc-linux-gnu --target=x86_64-pc-linux-gnu --enable-tests --disable-debug --without-toolchain-prefix --enable-debug-symbols MOZILLA_OFFICIAL= --disable-release ...

Thanks
Flags: needinfo?(mh+mozilla)
(Assignee)

Updated

a year ago
Depends on: 1363582
(Assignee)

Comment 5

a year ago
mozreview-review
Comment on attachment 8855846 [details]
Bug 1354581 - Move enable-release / MOZILLA_OFFICIAL support into the python configure

https://reviewboard.mozilla.org/r/127726/#review140896

::: build/moz.configure/toolchain.configure:970
(Diff revision 2)
>  set_define('HAVE_VISIBILITY_ATTRIBUTE',
>             depends(visibility_flags)(lambda v: bool(v) or None))
>  set_config('WRAP_SYSTEM_INCLUDES', wrap_system_includes)
>  set_config('VISIBILITY_FLAGS', visibility_flags)
>  
> +js_option(env='MOZILLA_OFFICIAL',

You should also move the AC_SUBST and AC_DEFINE from old-configure.

::: build/moz.configure/toolchain.configure:971
(Diff revision 2)
>             depends(visibility_flags)(lambda v: bool(v) or None))
>  set_config('WRAP_SYSTEM_INCLUDES', wrap_system_includes)
>  set_config('VISIBILITY_FLAGS', visibility_flags)
>  
> +js_option(env='MOZILLA_OFFICIAL',
> +          help='Build as Mozilla Official.')

Should probably find a better wording of what it means, although I don't have a good idea off the top of my head.

::: build/moz.configure/toolchain.configure:973
(Diff revision 2)
>  set_config('VISIBILITY_FLAGS', visibility_flags)
>  
> +js_option(env='MOZILLA_OFFICIAL',
> +          help='Build as Mozilla Official.')
> +
> +js_option('--enable-release', env='DEVELOPER_OPTIONS', default=False,

This line means that the DEVELOPER_OPTIONS env variable is an alternate way to set --enable-release, but --enable-release is supposed to *disable* DEVELOPER_OPTIONS.

::: build/moz.configure/toolchain.configure:977
(Diff revision 2)
> +
> +js_option('--enable-release', env='DEVELOPER_OPTIONS', default=False,
> +          help='Build with more conservative, release engineering-oriented options. This may slow down builds.')
> +
> +@depends('MOZILLA_OFFICIAL', "--enable-release")
> +@imports('os')

You're not using this. (I think the configure lint check would fail here)

::: build/moz.configure/toolchain.configure:978
(Diff revision 2)
> +js_option('--enable-release', env='DEVELOPER_OPTIONS', default=False,
> +          help='Build with more conservative, release engineering-oriented options. This may slow down builds.')
> +
> +@depends('MOZILLA_OFFICIAL', "--enable-release")
> +@imports('os')
> +def enable_release(target, mozilla_official_flags):

the argument names don't match the @depends.

::: build/moz.configure/toolchain.configure:979
(Diff revision 2)
> +          help='Build with more conservative, release engineering-oriented options. This may slow down builds.')
> +
> +@depends('MOZILLA_OFFICIAL', "--enable-release")
> +@imports('os')
> +def enable_release(target, mozilla_official_flags):
> +    # Official release, disable the developer mode

This comment seems like it's meant to go in the first_developer_options function.

::: build/moz.configure/toolchain.configure:980
(Diff revision 2)
> +
> +@depends('MOZILLA_OFFICIAL', "--enable-release")
> +@imports('os')
> +def enable_release(target, mozilla_official_flags):
> +    # Official release, disable the developer mode
> +    return len(mozilla_official_flags) or target

you want bool, not len. Also, why target?

::: build/moz.configure/toolchain.configure:983
(Diff revision 2)
> +def enable_release(target, mozilla_official_flags):
> +    # Official release, disable the developer mode
> +    return len(mozilla_official_flags) or target
> +
> +@depends(enable_release, target)
> +def first_developer_options(enable_release, target):

why first?

::: build/moz.configure/toolchain.configure:989
(Diff revision 2)
> +    return not enable_release
> +
> +set_config('DEVELOPER_OPTIONS', first_developer_options)
> +# Until we move all the DEVELOPER_OPTIONS consumers out of old-configure.
> +add_old_configure_assignment('DEVELOPER_OPTIONS',
> +                             not enable_release)

The previous comment about not enable_release not working applies to this as well.
Attachment #8855846 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 6

a year ago
mozreview-review
Comment on attachment 8855846 [details]
Bug 1354581 - Move enable-release / MOZILLA_OFFICIAL support into the python configure

https://reviewboard.mozilla.org/r/127726/#review140904

::: build/moz.configure/toolchain.configure:970
(Diff revision 2)
>  set_define('HAVE_VISIBILITY_ATTRIBUTE',
>             depends(visibility_flags)(lambda v: bool(v) or None))
>  set_config('WRAP_SYSTEM_INCLUDES', wrap_system_includes)
>  set_config('VISIBILITY_FLAGS', visibility_flags)
>  
> +js_option(env='MOZILLA_OFFICIAL',

MOZILLA_OFFICIAL should probably go somewhere else, too because it needs to be available on artifact builds. Maybe top-level moz.configure.
(Assignee)

Updated

a year ago
Flags: needinfo?(mh+mozilla)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 9

a year ago
mozreview-review-reply
Comment on attachment 8855846 [details]
Bug 1354581 - Move enable-release / MOZILLA_OFFICIAL support into the python configure

https://reviewboard.mozilla.org/r/127726/#review140896

> You should also move the AC_SUBST and AC_DEFINE from old-configure.

I also moved MOZ_INCLUDE_SOURCE_INFO in the python configure too.
(Assignee)

Comment 10

a year ago
mozreview-review
Comment on attachment 8855846 [details]
Bug 1354581 - Move enable-release / MOZILLA_OFFICIAL support into the python configure

https://reviewboard.mozilla.org/r/127726/#review143362

::: build/moz.configure/toolchain.configure:1005
(Diff revision 4)
>  def is_windows(target, host):
>      return host.kernel == 'WINNT' and target.kernel == 'WINNT'
>  
>  include('windows.configure', when=is_windows)
>  
> +js_option('--enable-release', env='DEVELOPER_OPTIONS', default=False,

With his, setting DEVELOPER_OPTIONS=1 is the same as --enable-release. That's the opposite of what it's supposed to be.

::: build/moz.configure/toolchain.configure:1008
(Diff revision 4)
>  include('windows.configure', when=is_windows)
>  
> +js_option('--enable-release', env='DEVELOPER_OPTIONS', default=False,
> +          help='Build with more conservative, release engineering-oriented options. This may slow down builds.')
> +
> +@depends("--enable-release", 'MOZILLA_OFFICIAL')

single quotes.

::: build/moz.configure/toolchain.configure:1009
(Diff revision 4)
>  
> +js_option('--enable-release', env='DEVELOPER_OPTIONS', default=False,
> +          help='Build with more conservative, release engineering-oriented options. This may slow down builds.')
> +
> +@depends("--enable-release", 'MOZILLA_OFFICIAL')
> +def enable_release(target, mozilla_official_flag, moz_include_source_info_flag):

This should fail because of the mismatch between the number of arguments here and in @depends.

::: build/moz.configure/toolchain.configure:1018
(Diff revision 4)
> +
> +@depends('MOZILLA_OFFICIAL')
> +def enable_official(mozilla_official_flag):
> +    # if we have the variable MOZILLA_OFFICIAL, MOZ_INCLUDE_SOURCE_INFO_FLAG
> +    # should be 1 too
> +    return bool(mozilla_official_flag)

You may want to add a "or None" to remove the variables entirely when MOZILLA_OFFICIAL is not set.

::: build/moz.configure/toolchain.configure:1020
(Diff revision 4)
> +def enable_official(mozilla_official_flag):
> +    # if we have the variable MOZILLA_OFFICIAL, MOZ_INCLUDE_SOURCE_INFO_FLAG
> +    # should be 1 too
> +    return bool(mozilla_official_flag)
> +
> +@depends(enable_release, target)

Please run trough try or run python/mozbuild/mozbuild/test/configure/lint.py locally. You'd find that target is unused and that the linter doesn't like that.

::: build/moz.configure/toolchain.configure:1023
(Diff revision 4)
> +    return bool(mozilla_official_flag)
> +
> +@depends(enable_release, target)
> +def dev_options(enable_release, target):
> +    # If official release, disable the developer mode
> +    return not bool(enable_release)

the only use of enable_release is in this function, you might as well remove enable_release, and make this function depend on --enable-release and 'MOZILLA_OFFICIAL' directly.

However, please note that the logic in old-configure is actually different. it is:

if !MOZILLA_OFFICIAL or !--enable-release

which is not the same as

if !(MOZILLA_OFFICIAL or --enable-release)

::: build/moz.configure/toolchain.configure:1030
(Diff revision 4)
> +set_config('MOZ_INCLUDE_SOURCE_INFO', enable_official)
> +# Until we move all the MOZ_INCLUDE_SOURCE_INFO out of old-configure.
> +add_old_configure_assignment('MOZ_INCLUDE_SOURCE_INFO',
> +                             enable_official)

Feels like this would be better in a separate patch.

::: moz.configure:74
(Diff revision 4)
>  @depends('--disable-tests')
>  def enable_tests(value):
>      if value:
>          return True
>  
> +js_option(env='MOZILLA_OFFICIAL',

You're missing a set_define for MOZILLA_OFFICIAL.

::: python/mozbuild/mozbuild/test/configure/test_moz_configure.py:64
(Diff revision 4)
> +        value = get_value_for(['--enable-release'],
> +                              environ={'MOZILLA_OFFICIAL': '',
> +                                       'MOZ_INCLUDE_SOURCE_INFO': '',
> +                                       'DEVELOPER_OPTIONS': ''})
> +
> +        self.assertEquals('--enable-release',
> +                          value)
> +
> +        value = get_value_for(
> +            environ={'MOZILLA_OFFICIAL': '1', 'MOZ_INCLUDE_SOURCE_INFO': '1',
> +                     'DEVELOPER_OPTIONS': ''})
> +
> +        self.assertEquals('MOZILLA_OFFICIAL=1',
> +                          value)

This test is there to test all_configure_options, I'm not sure it actually does the entire configure flag processing.

You should add a separate test. And test more combinations of --enable-release, MOZILLA_OFFICIAL and DEVELOPER_OPTIONS. test_toolkit_moz_configure.py would be a better location too.

Also, MOZ_INCLUDE_SOURCE_INFO is not an environment variable that configure recognizes with your changes, while old-configure would recognize it.
Attachment #8855846 - Flags: review?(mh+mozilla) → review-
(Reporter)

Updated

a year ago
Depends on: 1365530
Comment hidden (mozreview-request)
(Reporter)

Comment 12

a year ago
mozreview-review-reply
Comment on attachment 8855846 [details]
Bug 1354581 - Move enable-release / MOZILLA_OFFICIAL support into the python configure

https://reviewboard.mozilla.org/r/127726/#review143362

> the only use of enable_release is in this function, you might as well remove enable_release, and make this function depend on --enable-release and 'MOZILLA_OFFICIAL' directly.
> 
> However, please note that the logic in old-configure is actually different. it is:
> 
> if !MOZILLA_OFFICIAL or !--enable-release
> 
> which is not the same as
> 
> if !(MOZILLA_OFFICIAL or --enable-release)

I would prefer to keep it this way for now if that OK with you.
I am concern that I won't make it right the first time...

> This test is there to test all_configure_options, I'm not sure it actually does the entire configure flag processing.
> 
> You should add a separate test. And test more combinations of --enable-release, MOZILLA_OFFICIAL and DEVELOPER_OPTIONS. test_toolkit_moz_configure.py would be a better location too.
> 
> Also, MOZ_INCLUDE_SOURCE_INFO is not an environment variable that configure recognizes with your changes, while old-configure would recognize it.

Migrated into the other file
(Reporter)

Comment 13

a year ago
ping Mike?
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 14

a year ago
mozreview-review
Comment on attachment 8855846 [details]
Bug 1354581 - Move enable-release / MOZILLA_OFFICIAL support into the python configure

https://reviewboard.mozilla.org/r/127726/#review155986

There's still too much to comment about this patch that I feel like it's going to be a better use of everyone's time if I just do it. Sorry it took so long to come to that, though.
Attachment #8855846 - Flags: review?(mh+mozilla) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8855846 - Attachment is obsolete: true
Flags: needinfo?(mh+mozilla)
(Assignee)

Updated

a year ago
Assignee: sledru → mh+mozilla
(Assignee)

Comment 18

a year ago
And unsurprisingly, this busts artifact builds with:
  mozbuild.configure.options.InvalidOptionError: --enable-release is not available in this configuration
Comment hidden (mozreview-request)

Comment 20

a year ago
mozreview-review
Comment on attachment 8879776 [details]
Bug 1354581 - Move MOZILLA_OFFICIAL to python configure.

https://reviewboard.mozilla.org/r/151152/#review156928

::: js/src/old-configure.in
(Diff revision 1)
> -AC_SUBST(MOZILLA_OFFICIAL)
> -

I don't see this used meaningfully under js/src, but we can handle that later.
Attachment #8879776 - Flags: review?(cmanchester) → review+

Comment 21

a year ago
mozreview-review-reply
Comment on attachment 8879776 [details]
Bug 1354581 - Move MOZILLA_OFFICIAL to python configure.

https://reviewboard.mozilla.org/r/151152/#review156928

> I don't see this used meaningfully under js/src, but we can handle that later.

Nevermind, I think it is.

Comment 22

a year ago
mozreview-review
Comment on attachment 8879777 [details]
Bug 1354581 - Move --enable-release/DEVELOPER_OPTIONS to python configure.

https://reviewboard.mozilla.org/r/151154/#review156938

::: build/moz.configure/toolchain.configure:1062
(Diff revision 1)
> +set_config('DEVELOPER_OPTIONS', developer_options)
> +add_old_configure_assignment('DEVELOPER_OPTIONS', developer_options)

'DEVELOPER_OPTIONS' isn't AC_SUBST'd right now, is this `set_config` serving a purpose?
Attachment #8879777 - Flags: review?(cmanchester) → review+

Comment 23

a year ago
mozreview-review
Comment on attachment 8879778 [details]
Bug 1354581 - Add unit tests for DEVELOPER_OPTIONS.

https://reviewboard.mozilla.org/r/151156/#review156940
Attachment #8879778 - Flags: review?(cmanchester) → review+

Comment 24

a year ago
mozreview-review
Comment on attachment 8879784 [details]
Bug 1354581 - Make automation builds imply --enable-release at the configure level.

https://reviewboard.mozilla.org/r/151158/#review156942

The change seems fine, although it's pretty odd that most mozconfigs in the tree already set MOZILLA_OFFICIAL, which makes --enable-release redundant.
Attachment #8879784 - Flags: review?(cmanchester) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 29

a year ago
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/1c88753af1f2
Move MOZILLA_OFFICIAL to python configure. r=cmanchester+432261
https://hg.mozilla.org/integration/autoland/rev/ca42e8b81bbc
Move --enable-release/DEVELOPER_OPTIONS to python configure. r=cmanchester+432261
https://hg.mozilla.org/integration/autoland/rev/f1288dc02384
Add unit tests for DEVELOPER_OPTIONS. r=cmanchester+432261
https://hg.mozilla.org/integration/autoland/rev/65db6193ad97
Make automation builds imply --enable-release at the configure level. r=cmanchester+432261

Comment 31

a year ago
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/511c4f11a455
Add unit tests for DEVELOPER_OPTIONS. r=chmanchester
Keywords: leave-open

Comment 32

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1c88753af1f2
https://hg.mozilla.org/mozilla-central/rev/ca42e8b81bbc
https://hg.mozilla.org/mozilla-central/rev/65db6193ad97
https://hg.mozilla.org/mozilla-central/rev/511c4f11a455
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
status-firefox55: affected → wontfix
You need to log in before you can comment on or make changes to this bug.