Move --{enable,disable}-tests to Python configure

RESOLVED FIXED in Firefox 49

Status

Firefox Build System
General
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: Chris Manchester (mostly offline July 16-20), Assigned: Chris Manchester (mostly offline July 16-20))

Tracking

(Blocks: 1 bug)

unspecified
mozilla49

Firefox Tracking Flags

(firefox49 fixed)

Details

MozReview Requests

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

Attachments

(1 attachment)

Comment hidden (empty)
Created attachment 8753570 [details]
MozReview Request: Bug 1272530 - Move --disable-tests to Python configure. r=glandium

Review commit: https://reviewboard.mozilla.org/r/52878/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52878/
Attachment #8753570 - Flags: review?(mh+mozilla)
Comment on attachment 8753570 [details]
MozReview Request: Bug 1272530 - Move --disable-tests to Python configure. r=glandium

https://reviewboard.mozilla.org/r/52878/#review50216

::: moz.configure:86
(Diff revision 1)
> +@depends('--disable-compile-environment', '--disable-tests', '--help')
> +def test_defines_include(compile_env, enable_tests, help):
> +    if compile_env and enable_tests:
> +        return 'build/moz.configure/testdefines.configure'
> +
> +include(test_defines_include)

For one, it doesn't matter if those things are defined if --disable-compile-environment is used, so you might as well put the definitions here instead of using a conditionally included file.

Second, none of the AC_SUBST in that block your are moving are actually used. Only the AC_DEFINEs are, so you can skip the AC_SUBSTs.
Attachment #8753570 - Flags: review?(mh+mozilla)
https://reviewboard.mozilla.org/r/52878/#review50216

> For one, it doesn't matter if those things are defined if --disable-compile-environment is used, so you might as well put the definitions here instead of using a conditionally included file.
> 
> Second, none of the AC_SUBST in that block your are moving are actually used. Only the AC_DEFINEs are, so you can skip the AC_SUBSTs.

Setting _VARIADIC_MAX is gated on the compiler type in the original implementation, and we can't depend on c_compiler et al. when --disable-compile-environment is set.

I already didn't port the AC_SUBSTs for that reason (testdefines.configure as it is in this patch only uses set_define).
(In reply to Chris Manchester (:chmanchester) from comment #3)
> https://reviewboard.mozilla.org/r/52878/#review50216
> 
> > For one, it doesn't matter if those things are defined if --disable-compile-environment is used, so you might as well put the definitions here instead of using a conditionally included file.
> > 
> > Second, none of the AC_SUBST in that block your are moving are actually used. Only the AC_DEFINEs are, so you can skip the AC_SUBSTs.
> 
> Setting _VARIADIC_MAX is gated on the compiler type in the original
> implementation, and we can't depend on c_compiler et al. when
> --disable-compile-environment is set.

After some research, here's the good news: _VARIADIC_MAX was an implementation detail of simulated variadic templates in MSVC 2012. MSVC 2013 and 2015, which are the versions we support, have support for real variadic templates, and _VARIADIC_MAX is not necessary anymore.
> 
> I already didn't port the AC_SUBSTs for that reason (testdefines.configure
> as it is in this patch only uses set_define).

Err, sorry, somehow, I got confused.
Comment on attachment 8753570 [details]
MozReview Request: Bug 1272530 - Move --disable-tests to Python configure. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52878/diff/1-2/
Attachment #8753570 - Flags: review?(mh+mozilla)
Comment on attachment 8753570 [details]
MozReview Request: Bug 1272530 - Move --disable-tests to Python configure. r=glandium

https://reviewboard.mozilla.org/r/52878/#review51624

::: moz.configure:83
(Diff revision 2)
> +set_define('GTEST_USE_OWN_TR1_TUPLE',
> +           delayed_getattr(linux_gtest_defines, 'use_own_tr1_tuple'))

Note that now that android builds use libc++, this might not be needed anymore.

::: moz.configure
(Diff revision 2)
>      if compile_env:
>          return 'build/moz.configure/memory.configure'
>  
>  include(memory_include)
>  
> -

Please don't remove this line
Attachment #8753570 - Flags: review?(mh+mozilla) → review+

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d33cfe343732
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Updated

5 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.