Closed Bug 1354581 Opened 7 years ago Closed 7 years ago

Move enable-release / MOZILLA_OFFICIAL support into the python configure

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox55 wontfix, firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: Sylvestre, Assigned: glandium)

References

Details

Attachments

(4 files, 1 obsolete file)

Bug 1354581 - Move MOZILLA_OFFICIAL to python configure.
59 bytes, text/x-review-board-request
chmanchester
: review+
Details
Bug 1354581 - Move --enable-release/DEVELOPER_OPTIONS to python configure.
59 bytes, text/x-review-board-request
chmanchester
: review+
Details
Bug 1354581 - Add unit tests for DEVELOPER_OPTIONS.
59 bytes, text/x-review-board-request
chmanchester
: review+
Details
Bug 1354581 - Make automation builds imply --enable-release at the configure level.
59 bytes, text/x-review-board-request
chmanchester
: review+
Details 
      No description provided.
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)
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)
Depends on: 1363582
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-
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.
Flags: needinfo?(mh+mozilla)
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 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-
Depends on: 1365530
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
ping Mike?
Flags: needinfo?(mh+mozilla)
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-
Attachment #8855846 - Attachment is obsolete: true
Flags: needinfo?(mh+mozilla)
Assignee: sledru → mh+mozilla
And unsurprisingly, this busts artifact builds with:
  mozbuild.configure.options.InvalidOptionError: --enable-release is not available in this configuration
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 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 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 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 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+
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
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/511c4f11a455
Add unit tests for DEVELOPER_OPTIONS. r=chmanchester
Keywords: leave-open
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 56 → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: