1354581 - Move enable-release / MOZILLA_OFFICIAL support into the python configure

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1354581 - Move enable-release / MOZILLA_OFFICIAL support into the python configure

Review commit: https://reviewboard.mozilla.org/r/127726/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127726/

Comment 2

[Mike Hommey [:glandium]]

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`.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127726/diff/1-2/

Comment 4

[Sylvestre Ledru [:Sylvestre]]

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

Comment 5

[Mike Hommey [:glandium]]

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.

Comment 6

[Mike Hommey [:glandium]]

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.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127726/diff/2-3/

Comment 8

[Sylvestre Ledru [:Sylvestre]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127726/diff/3-4/

Comment 9

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 10

[Mike Hommey [:glandium]]

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.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127726/diff/4-5/

Comment 12

[Sylvestre Ledru [:Sylvestre]]

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

Comment 13

[Sylvestre Ledru [:Sylvestre]]

ping Mike?

Comment 14

[Mike Hommey [:glandium]]

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.

Comment 15

[Mike Hommey [:glandium]]

Attachment: Bug 1354581 - Move MOZILLA_OFFICIAL to python configure.

Review commit: https://reviewboard.mozilla.org/r/151152/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151152/

Comment 16

[Mike Hommey [:glandium]]

Attachment: Bug 1354581 - Move --enable-release/DEVELOPER_OPTIONS to python configure.

Review commit: https://reviewboard.mozilla.org/r/151154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151154/

Comment 17

[Mike Hommey [:glandium]]

Attachment: Bug 1354581 - Add unit tests for DEVELOPER_OPTIONS.

Review commit: https://reviewboard.mozilla.org/r/151156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151156/

Comment 18

[Mike Hommey [:glandium]]

And unsurprisingly, this busts artifact builds with:
  mozbuild.configure.options.InvalidOptionError: --enable-release is not available in this configuration

Comment 19

[Mike Hommey [:glandium]]

Attachment: Bug 1354581 - Make automation builds imply --enable-release at the configure level.

Review commit: https://reviewboard.mozilla.org/r/151158/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151158/

Comment 20

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 21

[Chris Manchester (limited bugmail, email directly)]

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

[Chris Manchester (limited bugmail, email directly)]

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?

Comment 23

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8879778 [details]
Bug 1354581 - Add unit tests for DEVELOPER_OPTIONS.

https://reviewboard.mozilla.org/r/151156/#review156940

Comment 24

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 8879776 [details]
Bug 1354581 - Move MOZILLA_OFFICIAL to python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151152/diff/1-2/

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 8879777 [details]
Bug 1354581 - Move --enable-release/DEVELOPER_OPTIONS to python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151154/diff/1-2/

Comment 27

[Mike Hommey [:glandium]]

Comment on attachment 8879778 [details]
Bug 1354581 - Add unit tests for DEVELOPER_OPTIONS.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151156/diff/1-2/

Comment 28

[Mike Hommey [:glandium]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151158/diff/1-2/

Comment 29

[Pulsebot]

Pushed by [e-mail]:
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 30

[Phil Ringnalda (:philor)]

Backed out f1288dc02384 and the test line from 65db6193ad97 in https://hg.mozilla.org/integration/autoland/rev/e80838b032bf04756aa6152b58b72e5ab8de7a3d for https://treeherder.mozilla.org/logviewer.html#?job_id=109416274&repo=autoland

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/511c4f11a455
Add unit tests for DEVELOPER_OPTIONS. r=chmanchester

Comment 32

[Carsten Book [:Tomcat]]

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