Closed Bug 1269517 Opened 8 years ago Closed 8 years ago

Move some header checks to Python configure

Categories

(Firefox Build System :: General, defect)

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

Tracking

(firefox50 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

(Blocks 2 open bugs)

Details

Attachments

(8 files)

Bug 1269517 - Fix various environment variables that may contain posix-style paths when invoking the js subconfigure.
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1269517 - Factor Python configure's try_preprocess into an invocation of a lower-level function.
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1269517 - Implement try_compile for Python configure.
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1269517 - Move android_platform to python configure.
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1269517 - Implement check_header in Python configure.
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1269517 - Move various header checks to Python configure.
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1269517 - Outfit the configure tests' fake compiler to handle compilation calls.
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1269517 - Remove header checks from old-configure that are no longer needed.
58 bytes, text/x-review-board-request
glandium
: review+
Details 
There are a handful of header checks in old-configure.in and js/src/old-configure.in that are pretty much isolated. This is a common and fairly straightforward check that will be used in a lot of places.
Comment on attachment 8768222 [details]
Bug 1269517 - Fix various environment variables that may contain posix-style paths when invoking the js subconfigure.

https://reviewboard.mozilla.org/r/62462/#review59678

Not having read the rest of the series, but in case I forget about it, this begs the question: how is it related to the rest?

::: build/subconfigure.py:218
(Diff revision 1)
> -        environ['PATH'] = os.environ['PATH']
> +        # These environment variables as passed from old-configure may contain
> +        # posix-style paths, which will not be meaningful to the js
> +        # subconfigure, which runs as a native python process, so use their
> +        # values from the environment. In the case of autoconf implemented
> +        # subconfigures, Msys will re-convert them properly.
> +        for var in ('HOME', 'LD_LIBRARY_PATH', 'TERM', 'PATH',

LD_LIBRARY_PATH is surprising for msys.
Attachment #8768222 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8768223 [details]
Bug 1269517 - Factor Python configure's try_preprocess into an invocation of a lower-level function.

https://reviewboard.mozilla.org/r/62464/#review59680

Commit message talks about tests. There are no tests.
Attachment #8768223 - Flags: review?(mh+mozilla)
Comment on attachment 8768224 [details]
Bug 1269517 - Implement try_compile for Python configure.

https://reviewboard.mozilla.org/r/62466/#review59682

::: build/moz.configure/compilechecks.configure:9
(Diff revision 1)
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +@template
> +@imports('textwrap')
> +def try_compile(body, language, flags=None, includes=None):

I think keeping the order includes/body from autoconf's AC_TRY_COMPILE makes sense.
I'd default language to "C++" so that we don't have to pass it most of the time (we're mostly compiling C++ code, it makes sense to do the checks for C++ by default).

It might be convenient to allow passing a tuple/list of languages, too, so that both C and C++ can be handled.

That being said, I had a different vision for those checks, but meh, we can refactor later.

::: build/moz.configure/compilechecks.configure:11
(Diff revision 1)
> +    def quote_include(path):
> +        if ('<', '>') == (path[0], path[-1]):
> +            return path
> +        return '"%s"' % path

I'm pretty sure you don't have to worry about this. Just use <> everywhere.

::: build/moz.configure/compilechecks.configure:16
(Diff revision 1)
> +    def quote_include(path):
> +        if ('<', '>') == (path[0], path[-1]):
> +            return path
> +        return '"%s"' % path
> +    source_lines = ['#include %s' % quote_include(f) for f in includes]
> +    source = '\n'.join(source_lines) + '\n'

the first line feed doesn't seem very useful.

::: build/moz.configure/compilechecks.configure:21
(Diff revision 1)
> +    source = '\n'.join(source_lines) + '\n'
> +    source += textwrap.dedent('''\
> +        int
> +        main(void)
> +        {
> +        %s

You'll want to add a ; when there is none at the end of "body".

::: build/moz.configure/compilechecks.configure:26
(Diff revision 1)
> +        %s
> +          return 0;
> +        }
> +    ''' % body)
> +    flags = flags or []
> +    flags.insert(0, '-c')

you might as well put it last with flags.append, it shouldn't matter.

::: python/mozbuild/mozbuild/test/configure/test_header_checks.py:23
(Diff revision 1)
> +
> +class TestHeaderChecks(unittest.TestCase):
> +
> +    def get_mock_compiler(self, expected_test_content=None, expected_flags=None):
> +        expected_flags = expected_flags or []
> +        def mock_compiler(stdin, args):

It would be better to augment FakeCompiler instead.

::: python/mozbuild/mozbuild/test/configure/test_header_checks.py:35
(Diff revision 1)
> +                with open(test_file) as fh:
> +                    test_content = fh.read()
> +                self.assertEqual(test_content, expected_test_content)
> +
> +            status = 0
> +            if '--error' in args:

something like -funknown-flag would somehow more realistic
Attachment #8768224 - Flags: review?(mh+mozilla)
Comment on attachment 8768225 [details]
Bug 1269517 - Move android_platform to python configure.

https://reviewboard.mozilla.org/r/62468/#review59688

This doesn't seem very related.

::: build/moz.configure/android-ndk.configure:28
(Diff revision 1)
> +          help='android platform version',
> +          default=min_android_version)
> +
> +@depends('--with-android-version', min_android_version)
> +def android_version(value, min_value):
> +    if value[0] < min_value:

You're comparing strings. '15' < '9' is True
There should also be some type checking.

::: build/moz.configure/android-ndk.configure:49
(Diff revision 1)
> +@checking('for android platform directory')
> +def android_platform(target, android_version, ndk):
> +    if target.os != 'Android':
> +        return
> +
> +    if '86' in target.cpu:

this condition is true for x86_64, which didn't match i?86 in the previous check. You want to check target.cpu == 'x86', which is the value for target_dir_name, so it can just fall back to the target_dir_name = target.cpu case :)
Attachment #8768225 - Flags: review?(mh+mozilla)
Comment on attachment 8768226 [details]
Bug 1269517 - Implement check_header in Python configure.

https://reviewboard.mozilla.org/r/62470/#review59696

::: build/moz.configure/compilechecks.configure:46
(Diff revision 1)
>                                     language, source, flags,
>                                     onerror=lambda: None)
>      return check
> +
> +@template
> +def check_header(header, language='C', flags=None, includes=None):

I know a lot of our checks are with the C compiler today, but we should default to C++

::: build/moz.configure/compilechecks.configure:60
(Diff revision 1)
> +        @depends_when(check_header(header, **kwargs), when=when)
> +        @checking('for %s' % header)

This will only print the checking message after having actually checked. Let me give this more thought. There are things I wanted to improve related to @depends and possibly other decorators, I'm wondering if that would help here or not.
Attachment #8768226 - Flags: review?(mh+mozilla)
https://reviewboard.mozilla.org/r/62462/#review59678

Munging of INCLUDE makes the js subconfigure fail to find headers, and munging of TEMP makes msvc called from js subconfigure fail to compile files.
https://reviewboard.mozilla.org/r/62468/#review59688

I'm sure this is clear from the next patch, but we need to pass `-idirafter $platform_dir/usr/include` to find headers on android.
https://reviewboard.mozilla.org/r/62462/#review59678

> LD_LIBRARY_PATH is surprising for msys.

I thought I saw docs about this, but that was for cygwin. I don't think it's relevant here, so we can leave it out.
https://reviewboard.mozilla.org/r/62470/#review59696

> This will only print the checking message after having actually checked. Let me give this more thought. There are things I wanted to improve related to @depends and possibly other decorators, I'm wondering if that would help here or not.

Yeah, I'm actually not sure how to do this better in the current set up...
So, if you change try_compile to be a decorator that just wraps the function it's given, you can then do:

@checking('for ...')
@try_compile(...)
def check(compiler_result):
    ...

And the checking should be printed at the right time.
(In reply to Mike Hommey [:glandium] from comment #16)
> So, if you change try_compile to be a decorator that just wraps the function
> it's given, you can then do:
> 
> @checking('for ...')
> @try_compile(...)
> def check(compiler_result):
>     ...
> 
> And the checking should be printed at the right time.

That didn't quite work, but I found another way.
https://reviewboard.mozilla.org/r/62466/#review59682

> the first line feed doesn't seem very useful.

I'm not sure which line feed is meant here. We end up with one between each line, and one at the end (which looks right to me).
https://reviewboard.mozilla.org/r/62466/#review59682

> I'm not sure which line feed is meant here. We end up with one between each line, and one at the end (which looks right to me).

It looks like I misread.
Review commit: https://reviewboard.mozilla.org/r/64384/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64384/
Attachment #8771576 - Flags: review?(mh+mozilla)
Attachment #8768223 - Flags: review?(mh+mozilla)
Attachment #8768224 - Flags: review?(mh+mozilla)
Attachment #8768225 - Flags: review?(mh+mozilla)
Attachment #8768226 - Flags: review?(mh+mozilla)
Comment on attachment 8768222 [details]
Bug 1269517 - Fix various environment variables that may contain posix-style paths when invoking the js subconfigure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62462/diff/1-2/
Comment on attachment 8768223 [details]
Bug 1269517 - Factor Python configure's try_preprocess into an invocation of a lower-level function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62464/diff/1-2/
Comment on attachment 8768224 [details]
Bug 1269517 - Implement try_compile for Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62466/diff/1-2/
Comment on attachment 8768225 [details]
Bug 1269517 - Move android_platform to python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62468/diff/1-2/
Comment on attachment 8768226 [details]
Bug 1269517 - Implement check_header in Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62470/diff/1-2/
Comment on attachment 8768227 [details]
Bug 1269517 - Move various header checks to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62472/diff/1-2/
Attachment #8768223 - Flags: review?(mh+mozilla)
Comment on attachment 8768223 [details]
Bug 1269517 - Factor Python configure's try_preprocess into an invocation of a lower-level function.

https://reviewboard.mozilla.org/r/62464/#review62130

Previous review comment still applies: The commit message talks about tests, but there are no tests added by this patch.
Attachment #8771576 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8771576 [details]
Bug 1269517 - Outfit the configure tests' fake compiler to handle compilation calls.

https://reviewboard.mozilla.org/r/64384/#review62132
Comment on attachment 8768224 [details]
Bug 1269517 - Implement try_compile for Python configure.

https://reviewboard.mozilla.org/r/62466/#review62134

::: build/moz.configure/compilechecks.configure:27
(Diff revisions 1 - 2)
> +            @checking(check_msg, callback=lambda r: r is not None)
> +            def wrapper(*args, **kwargs):
> +                return fn(*args, **kwargs)
> +            return wrapper

this can be written as return checking(check_msg, ...)(fn)

::: build/moz.configure/compilechecks.configure:9
(Diff revision 2)
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +@template
> +@imports('textwrap')
> +def try_compile(includes=None, body='', language='C++', flags=None, check_msg=None):

A comment describing what the function does and its inputs would be nice.

I'm not super fan of the check_msg thing, but let's go with this for now.
Attachment #8768224 - Flags: review?(mh+mozilla) → review+
Attachment #8768225 - Flags: review?(mh+mozilla)
Comment on attachment 8768225 [details]
Bug 1269517 - Move android_platform to python configure.

https://reviewboard.mozilla.org/r/62468/#review62136

::: build/moz.configure/android-ndk.configure:17
(Diff revision 2)
> +@depends('--help')
> +def min_android_version(_):
> +    return '9'
> +
> +js_option('--with-android-version',
> +          nargs=1,
> +          help='android platform version',
> +          default=min_android_version)
> +
> +@depends('--with-android-version', min_android_version)
> +@imports(_from='__builtin__', _import='ValueError')
> +def android_version_number(value, min_version):
> +    try:
> +        int(value[0])
> +    except ValueError:
> +        die('--with-android-version expects an integer value')
> +
> +    if Version(value[0]) < Version(min_version):
> +        die('--with-android-version must be at least %s (got %s)',
> +            min_version, value[0])
> +
> +    return int(value[0])

You could return a literal int from min_android_version, and then do something like:

try:
    version = int(value[0])
except ValueError:
   ...

if version < min_version:
   ...

return version

Note you're not handling the case where value is a NegativeOptionValue, in which case value[0] will throw an IndexError.
Comment on attachment 8768226 [details]
Bug 1269517 - Implement check_header in Python configure.

https://reviewboard.mozilla.org/r/62470/#review62142

::: build/moz.configure/compilechecks.configure:66
(Diff revision 2)
> +
> +    return try_compile(includes=includes, language=language, flags=flags,
> +                       check_msg='for %s' % header)
> +
> +@template
> +def check_headers(*headers, **kwargs):

Come to think of it, I'm not sure if it's desirable to have both check_header and  check_headers. Especially when they don't have the same behavior. If we do want to have both, then check_headers should only be a helper to loop over check_header, and check_header should be the one setting the defines.

::: python/mozbuild/mozbuild/test/configure/test_header_checks.py:192
(Diff revision 2)
> +            return 0;
> +          }
> +        ''')
> +
> +        cmd = textwrap.dedent('''\
> +           (have_foo,) = check_headers('foo.h', includes=['std.h', 'bar.h'])

This seems like something we should explicitly forbid as not making sense.
Attachment #8768226 - Flags: review?(mh+mozilla)
Comment on attachment 8768227 [details]
Bug 1269517 - Move various header checks to Python configure.

https://reviewboard.mozilla.org/r/62472/#review62144

::: build/moz.configure/headers.configure:10
(Diff revision 2)
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# Check for headers defining standard int types.
> +have_stdint, have_inttypes = check_headers('stdint.h', 'inttypes.h')
> +
> +set_config('HAVE_INTTYPES_H', have_inttypes)

I'm skeptical. HAVE_INTTYPES_H should already be set from just using check_headers('inttypes.h'), right?

::: build/moz.configure/headers.configure:25
(Diff revision 2)
> +
> +add_old_configure_assignment('HAVE_MALLOC_H', have_malloc)
> +
> +check_headers(
> +    'sys/byteorder.h',
> +    'compat.h',

there is no HAVE_COMPAT_H in the tree

::: build/moz.configure/headers.configure:27
(Diff revision 2)
> +
> +check_headers(
> +    'sys/byteorder.h',
> +    'compat.h',
> +    'getopt.h',
> +    'sys/bitypes.h',

There is no HAVE_SYS_BITYPES_H in the tree.

::: build/moz.configure/headers.configure:28
(Diff revision 2)
> +check_headers(
> +    'sys/byteorder.h',
> +    'compat.h',
> +    'getopt.h',
> +    'sys/bitypes.h',
> +    'memory.h',

Nothing in the tree appears to be using HAVE_MEMORY_H from here.

::: build/moz.configure/headers.configure:30
(Diff revision 2)
> +    'compat.h',
> +    'getopt.h',
> +    'sys/bitypes.h',
> +    'memory.h',
> +    'unistd.h',
> +    'gnu/libc-version.h',

The last actual use of HAVE_GNU_GET_LIBC_VERSION was removed in http://hg.mozilla.org/mozilla-central/rev/8d6db7f8fe09 (there is still a ifdef, but it can be removed)

::: build/moz.configure/headers.configure:32
(Diff revision 2)
> +    'sys/bitypes.h',
> +    'memory.h',
> +    'unistd.h',
> +    'gnu/libc-version.h',
> +    'nl_types.h',
> +    'X11/XKBlib.h',

Nothing is using HAVE_X11_XKBLIB_H

::: build/moz.configure/headers.configure:33
(Diff revision 2)
> +    'memory.h',
> +    'unistd.h',
> +    'gnu/libc-version.h',
> +    'nl_types.h',
> +    'X11/XKBlib.h',
> +    'io.h',

funny thing: the two only places in gecko code where HAVE_IO_H are using it for isatty. isatty defined in io.h is a windows thing, and we set HAVE_IO_H manually on Windows.

::: build/moz.configure/headers.configure:50
(Diff revision 2)
> +)
> +
> +# Quota support
> +check_headers(
> +    'sys/quota.h',
> +    'sys/sysmacros.h',

Nothing uses HAVE_SYS_SYSMACROS_H

::: build/moz.configure/headers.configure:54
(Diff revision 2)
> +check_headers('linux/quota.h',
> +              includes=['sys/socket.h'],
> +              when=non_msvc_compiler)
> +
> +# SCTP support - needs various network include headers
> +check_headers(
> +    'linux/if_addr.h',
> +    'linux/rtnetlink.h',

Everything that starts with linux/ could be limited to target.kernel == 'Linux'

::: build/moz.configure/headers.configure:82
(Diff revision 2)
> +# Checks for headers relevant when building js.
> +
> +endian_h, sys_isa_defs_h, sys_cdefs_h = check_headers(
> +    'endian.h',
> +    'sys/isa_defs.h',
> +    'sys/cdefs.h',

Nothing uses this one.

::: build/moz.configure/headers.configure:86
(Diff revision 2)
> +set_define('JS_HAVE_ENDIAN_H', endian_h)
> +set_define('JS_HAVE_SYS_ISA_DEFS_H', sys_isa_defs_h)
> +
> +(machine_endian_h,) = check_headers(
> +    'machine/endian.h',
> +    includes=['sys/types.h'],
> +    when=building_js_non_msvc,
> +)
> +
> +set_define('JS_HAVE_MACHINE_ENDIAN_H', machine_endian_h)

All these can go away entirely (reason being that all the compilers we support don't need those headers anymore, and we can actually remove our uses of them ; I actually have something related to that in a local branch ; I'll file a separate bug about it), so it's not worth moving them.

::: moz.configure:110
(Diff revision 2)
> +@depends('--disable-compile-environment', '--help')
> +def headers_check_include(compile_env, help):
> +    if compile_env:
> +        return 'build/moz.configure/headers.configure'
> +
> +include(headers_check_include)

We're starting to have way too many of those, please file a followup to add a template (probably) around this hack.

::: old-configure.in
(Diff revision 2)
> -dnl These are all the places some variant of statfs can be hiding.
> -MOZ_CHECK_HEADERS(sys/statvfs.h sys/statfs.h sys/vfs.h sys/mount.h)
> -
> -dnl Quota support
> -MOZ_CHECK_HEADERS(sys/quota.h sys/sysmacros.h)
> -MOZ_CHECK_HEADERS([linux/quota.h],,,[#include <sys/socket.h>])

okay, forget what I said about the includes argument to checking_headers not making sense.
Attachment #8768227 - Flags: review?(mh+mozilla)
https://reviewboard.mozilla.org/r/62468/#review62136

> You could return a literal int from min_android_version, and then do something like:
> 
> try:
>     version = int(value[0])
> except ValueError:
>    ...
> 
> if version < min_version:
>    ...
> 
> return version
> 
> Note you're not handling the case where value is a NegativeOptionValue, in which case value[0] will throw an IndexError.

I'm using min_android_version as a default, and I guess we don't allow ints as defaults...

It's a little suprising we can end up with a NegativeOptionValue we have nargs=1 and a default provided. Can we reject this earlier?
https://reviewboard.mozilla.org/r/62470/#review62142

> Come to think of it, I'm not sure if it's desirable to have both check_header and  check_headers. Especially when they don't have the same behavior. If we do want to have both, then check_headers should only be a helper to loop over check_header, and check_header should be the one setting the defines.

Agreed, although there are a couple of places we have MOZ_CHECK_HEADER called today where we have the result of checking for the header but not setting the define. But there are only a couple, and I haven't ensured that's actually a useful behavior.

> This seems like something we should explicitly forbid as not making sense.

I'm not sure what you mean, is this the comment you said to ignore on the next patch?
https://reviewboard.mozilla.org/r/62472/#review62144

> I'm skeptical. HAVE_INTTYPES_H should already be set from just using check_headers('inttypes.h'), right?

It's already set as a define by check_headers, but it's also checked in moz.build files.

> We're starting to have way too many of those, please file a followup to add a template (probably) around this hack.

Filed bug 1287924
(In reply to Chris Manchester (:chmanchester) from comment #34)
> I'm not sure what you mean, is this the comment you said to ignore on the
> next patch?

Yes
(In reply to Chris Manchester (:chmanchester) from comment #33)
> I'm using min_android_version as a default, and I guess we don't allow ints
> as defaults...

You can still do

try:
    version = int(value[0])
except ValueError:
    ...

if version < int(min_version):
    ...

return version


> It's a little suprising we can end up with a NegativeOptionValue we have
> nargs=1 and a default provided. Can we reject this earlier?

Unfortunately, not now. There are many legitimate uses of disable-able options with nargs=1 with a default. We'd need some argument telling "this can't be disabled". But it's probably simpler to explicitly reject the case with a `if not value: die()`
Comment on attachment 8768222 [details]
Bug 1269517 - Fix various environment variables that may contain posix-style paths when invoking the js subconfigure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62462/diff/2-3/
Attachment #8768223 - Flags: review?(mh+mozilla)
Attachment #8768225 - Flags: review?(mh+mozilla)
Attachment #8768226 - Flags: review?(mh+mozilla)
Attachment #8768227 - Flags: review?(mh+mozilla)
Comment on attachment 8768223 [details]
Bug 1269517 - Factor Python configure's try_preprocess into an invocation of a lower-level function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62464/diff/2-3/
Comment on attachment 8771576 [details]
Bug 1269517 - Outfit the configure tests' fake compiler to handle compilation calls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64384/diff/1-2/
Comment on attachment 8768224 [details]
Bug 1269517 - Implement try_compile for Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62466/diff/2-3/
Comment on attachment 8768225 [details]
Bug 1269517 - Move android_platform to python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62468/diff/2-3/
Comment on attachment 8768226 [details]
Bug 1269517 - Implement check_header in Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62470/diff/2-3/
Comment on attachment 8768227 [details]
Bug 1269517 - Move various header checks to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62472/diff/2-3/
Attachment #8768223 - Flags: review?(mh+mozilla)
Comment on attachment 8768223 [details]
Bug 1269517 - Factor Python configure's try_preprocess into an invocation of a lower-level function.

https://reviewboard.mozilla.org/r/62464/#review62766

Previous review comment still applies: The commit message talks about tests, but there are no tests added by this patch.
https://reviewboard.mozilla.org/r/62466/#review62768

::: build/moz.configure/compilechecks.configure:12
(Diff revisions 2 - 3)
> +
> +# Generates a test program and attempts to compile it. In case of failure, the
> +# resulting check will return None. If the test program succeeds, it will return
> +# the output of the test program.
> +# - `includes` are the includes that will appear at the top of the generated
> +#   test program.

worth mentioning it's only file names, not full #include syntax.

::: build/moz.configure/compilechecks.configure:13
(Diff revisions 2 - 3)
> +# Generates a test program and attempts to compile it. In case of failure, the
> +# resulting check will return None. If the test program succeeds, it will return
> +# the output of the test program.
> +# - `includes` are the includes that will appear at the top of the generated
> +#   test program.
> +# - `body` is the code that will appear in the main function of the generated

worth mentioning that "return 0" is automatically added at the end of the code.
Attachment #8768225 - Flags: review?(mh+mozilla) → review+
Attachment #8768226 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8768226 [details]
Bug 1269517 - Implement check_header in Python configure.

https://reviewboard.mozilla.org/r/62470/#review62772

It strikes me that in most cases, we could actually get away with only running the preprocessor, not a complete compilation. I'm okay with leaving that to a followup, though, since autoconf does use a compile for those tests. Also, we're going to start needing a cache. Followup too.

::: build/moz.configure/compilechecks.configure:64
(Diff revision 3)
>                                     language, source, flags,
>                                     onerror=lambda: None)
>      return check
> +
> +@template
> +def check_header(header, language='C++', flags=None, includes=None, when=None):

For both this and check_header a comment explaining what it does and its input/output would be nice.

::: python/mozbuild/mozbuild/test/configure/test_header_checks.py:175
(Diff revision 3)
> +        ''')
> +
> +        config, out, status = self.do_compile_test(cmd,
> +                                                   expected_test_content=expected_test_content)
> +        self.assertEqual(status, 0)
> +        self.assertEqual(config, {'DEFINES': {'HAVE_FOO_H': True}})

you could check the out like on line 206.
Comment on attachment 8768227 [details]
Bug 1269517 - Move various header checks to Python configure.

https://reviewboard.mozilla.org/r/62472/#review62776

It would be better to split this in two patches: one that removes the checks that are useless (with a commit message saying why), and another that moves the remainder to python configure.

::: build/moz.configure/headers.configure:8
(Diff revision 3)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# Check for headers defining standard int types.
> +have_stdint, have_inttypes = check_headers('stdint.h', 'inttypes.h')

you could replace "have_stdint" with "_"

::: build/moz.configure/headers.configure:24
(Diff revision 3)
> +add_old_configure_assignment('HAVE_MALLOC_H', have_malloc)
> +
> +check_headers(
> +    'sys/byteorder.h',
> +    'getopt.h',
> +    'memory.h',

cf. comment 32.

::: js/src/old-configure.in
(Diff revision 3)
> -MOZ_CHECK_HEADERS(endian.h)
> -if test "$ac_cv_header_endian_h" = yes; then
> -    AC_DEFINE(JS_HAVE_ENDIAN_H)
> -fi
> -
> -MOZ_CHECK_HEADERS([machine/endian.h],[],[],[#include <sys/types.h>])
> -if test "$ac_cv_header_machine_endian_h" = yes; then
> -    AC_DEFINE(JS_HAVE_MACHINE_ENDIAN_H)
> -fi
> -
> -MOZ_CHECK_HEADERS(sys/isa_defs.h)
> -if test "$ac_cv_header_sys_isa_defs_h" = yes; then
> -    AC_DEFINE(JS_HAVE_SYS_ISA_DEFS_H)
> -fi
> -

Please leave those for bug 1287671.
Attachment #8768227 - Flags: review?(mh+mozilla)
https://reviewboard.mozilla.org/r/62472/#review62776

> you could replace "have_stdint" with "_"

... or use check_header twice.
(In reply to Mike Hommey [:glandium] from comment #45)
> Comment on attachment 8768223 [details]
> Bug 1269517 - Factor Python configure's try_preprocess into an invocation of
> a lower-level function.
> 
> https://reviewboard.mozilla.org/r/62464/#review62766
> 
> Previous review comment still applies: The commit message talks about tests,
> but there are no tests added by this patch.

Wow, I missed this comment repeatedly, sorry about that. The mozreview UI seems to bury a comment like this if an issue isn't also opened.
This patch removes the checks for compat.h, sys/bittypes.h, gnu/libc-version.h,
X11/XKBlib.h, sys/sysmacros.h, and sys/cdefs.h from old-configure, because they
are not checked meaningfully in the tree.

The check for memory.h is removed, because while there are checks for
HAVE_MEMORY_H in the tree, they are in places this is set by a third party
build system.

The check for io.h is also removed, because while there are checks for
HAVE_IO_H, they're only relevant on windows, where this is set manually in
old-configure.

Review commit: https://reviewboard.mozilla.org/r/66526/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66526/
Attachment #8773870 - Flags: review?(mh+mozilla)
Attachment #8768223 - Flags: review?(mh+mozilla)
Attachment #8768227 - Flags: review?(mh+mozilla)
Comment on attachment 8768222 [details]
Bug 1269517 - Fix various environment variables that may contain posix-style paths when invoking the js subconfigure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62462/diff/3-4/
Comment on attachment 8768223 [details]
Bug 1269517 - Factor Python configure's try_preprocess into an invocation of a lower-level function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62464/diff/3-4/
Comment on attachment 8771576 [details]
Bug 1269517 - Outfit the configure tests' fake compiler to handle compilation calls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64384/diff/2-3/
Comment on attachment 8768224 [details]
Bug 1269517 - Implement try_compile for Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62466/diff/3-4/
Comment on attachment 8768225 [details]
Bug 1269517 - Move android_platform to python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62468/diff/3-4/
Comment on attachment 8768226 [details]
Bug 1269517 - Implement check_header in Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62470/diff/3-4/
Comment on attachment 8768227 [details]
Bug 1269517 - Move various header checks to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62472/diff/3-4/
Comment on attachment 8768223 [details]
Bug 1269517 - Factor Python configure's try_preprocess into an invocation of a lower-level function.

https://reviewboard.mozilla.org/r/62464/#review63910

::: build/moz.configure/util.configure:91
(Diff revision 4)
>  
> +@imports('os')
> +@imports('subprocess')
> +@imports(_from='mozbuild.configure.util', _import='LineIO')
> +@imports(_from='tempfile', _import='mkstemp')
> +def try_invoke_compiler(compiler, language, source, flags=None, onerror=None):

Note, I'm not convinced this needs to move to util.configure.
Attachment #8768223 - Flags: review?(mh+mozilla)
Comment on attachment 8773870 [details]
Bug 1269517 - Remove header checks from old-configure that are no longer needed.

https://reviewboard.mozilla.org/r/66526/#review63912
Attachment #8773870 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8768223 [details]
Bug 1269517 - Factor Python configure's try_preprocess into an invocation of a lower-level function.

https://reviewboard.mozilla.org/r/62464/#review63914
Attachment #8768223 - Flags: review+
Comment on attachment 8768227 [details]
Bug 1269517 - Move various header checks to Python configure.

https://reviewboard.mozilla.org/r/62472/#review63916
Attachment #8768227 - Flags: review?(mh+mozilla) → review+
https://reviewboard.mozilla.org/r/62464/#review63910

> Note, I'm not convinced this needs to move to util.configure.

For these tests it's more convenient to include util.configure than toolchain.configure. There's probably a better way to structure these tests, I can look into it as a follow up.
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8457e2d24535
Fix various environment variables that may contain posix-style paths when invoking the js subconfigure. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3b5b77d0dcc
Factor Python configure's try_preprocess into an invocation of a lower-level function. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/c12838ecfbb5
Outfit the configure tests' fake compiler to handle compilation calls. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/de64f0094103
Implement try_compile for Python configure. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd2ca7c770fd
Move android_platform to python configure. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/2081a003f2d8
Implement check_header in Python configure. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/b50c01f263a5
Remove header checks from old-configure that are no longer needed. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/550fc29355f1
Move various header checks to Python configure. r=glandium
Depends on: 1289691
Blocks: 1290026
No longer blocks: 1290026
Depends on: 1290305
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: