Closed Bug 1294803 Opened 8 years ago Closed 8 years ago

Move ctypes and libffi related variables to moz.configure

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Namely, BUILD_CTYPES and MOZ_SYSTEM_FFI.
Comment on attachment 8781247 [details]
Bug 1294803 - Move MOZ_SYSTEM_FFI to moz.configure in preparation for moving libffi to our build system.

https://reviewboard.mozilla.org/r/71680/#review69266

::: build/moz.configure/ffi.configure:12
(Diff revision 1)
> +@depends_when(c_compiler, when=use_system_ffi)
> +def system_ffi_spec(info):
> +    # Vanilla libffi 3.0.9 needs a few patches from upcoming version 3.0.10
> +    # for non-GCC compilers.
> +    if info.type == 'gcc':
> +        return 'libffi >= 3.0.9'
> +    return 'libffi > 3.0.9'

Part of the solution for you, though, might be to say that we don't need to care about gcc vs. non-gcc anymore. 3.0.10 was released 5 years ago.

::: build/moz.configure/ffi.configure:20
(Diff revision 1)
> +    # for non-GCC compilers.
> +    if info.type == 'gcc':
> +        return 'libffi >= 3.0.9'
> +    return 'libffi > 3.0.9'
> +
> +system_ffi = pkg_check_modules('MOZ_FFI', system_ffi_spec, use_system_ffi)

random thought: this would be clearer with "condition=use_system_ffi", which makes me wonder why it's "condition" and not "when", like for other things.

::: moz.configure:104
(Diff revision 1)
> +include_when('build/moz.configure/ffi.configure',
> +             when='--enable-compile-environment')

This should only be included from js/moz.configure. OTOH --enable-compile-environment and toolchain tests are only available after js/moz.configure is included :-/
I don't have a solution here, but that's something that is hitting me for other things as well, so this is something we need to find a solution for.
Attachment #8781247 - Flags: review?(mh+mozilla)
Comment on attachment 8781248 [details]
Bug 1294803 - Move BUILD_CTYPES to Python configure.

https://reviewboard.mozilla.org/r/71682/#review69268
Attachment #8781248 - Flags: review?(mh+mozilla)
Comment on attachment 8781248 [details]
Bug 1294803 - Move BUILD_CTYPES to Python configure.

https://reviewboard.mozilla.org/r/71682/#review69270
Attachment #8781248 - Flags: review+
Comment on attachment 8781247 [details]
Bug 1294803 - Move MOZ_SYSTEM_FFI to moz.configure in preparation for moving libffi to our build system.

https://reviewboard.mozilla.org/r/71680/#review69266

> random thought: this would be clearer with "condition=use_system_ffi", which makes me wonder why it's "condition" and not "when", like for other things.

`pkg_check_modules` pre-dates `when`, the inconsistency should just be fixed.

> This should only be included from js/moz.configure. OTOH --enable-compile-environment and toolchain tests are only available after js/moz.configure is included :-/
> I don't have a solution here, but that's something that is hitting me for other things as well, so this is something we need to find a solution for.

Doing these checks in the top-level configure is status-quo (for now). Agree about the issue in general.
Comment on attachment 8781247 [details]
Bug 1294803 - Move MOZ_SYSTEM_FFI to moz.configure in preparation for moving libffi to our build system.

https://reviewboard.mozilla.org/r/71680/#review69712

Please file a followup to move the ffi.configure include to js/.
Please also add a comment in the commit message wrt always requiring ffi > 3.0.9 because 3.0.10 is already 5 years old.
Attachment #8781247 - Flags: review?(mh+mozilla) → review+
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e6aae0b93d
Move MOZ_SYSTEM_FFI to moz.configure in preparation for moving libffi to our build system. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f6047b2bc09
Move BUILD_CTYPES to Python configure. r=glandium
https://hg.mozilla.org/mozilla-central/rev/c0e6aae0b93d
https://hg.mozilla.org/mozilla-central/rev/7f6047b2bc09
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
This breaks artifact builds, because we no longer set BUILD_CTYPES when --disable-compile-environment, and we use that to check whether to recurse into any ctypes related directories. The resulting build fails to find ctypes.jsm during startup. I'll back this out.
Grr...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/ddc877cac511
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8781247 - Flags: review+ → review?(mh+mozilla)
Attachment #8781248 - Flags: review+ → review?(mh+mozilla)
These patches address comment 12 by putting BUILD_CTYPES in the root moz.configure.
Comment on attachment 8781248 [details]
Bug 1294803 - Move BUILD_CTYPES to Python configure.

https://reviewboard.mozilla.org/r/71682/#review71308

::: moz.configure:106
(Diff revision 3)
>               when='--enable-compile-environment')
>  include_when('build/moz.configure/warnings.configure',
>               when='--enable-compile-environment')
>  
> +
> +@depends(building_js, '--help')

Might as well put it in js/moz.configure.
Attachment #8781248 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8781247 [details]
Bug 1294803 - Move MOZ_SYSTEM_FFI to moz.configure in preparation for moving libffi to our build system.

https://reviewboard.mozilla.org/r/71680/#review71312

::: moz.configure:128
(Diff revision 3)
>  
>  set_config('JS_HAS_CTYPES', js_has_ctypes)
>  set_define('JS_HAS_CTYPES', js_has_ctypes)
>  add_old_configure_assignment('JS_HAS_CTYPES', js_has_ctypes)
>  
> +@depends('--enable-ctypes', '--enable-compile-environment', '--help')

So, here we have a reason why it wouldn't work in js/moz.configure... until bug 1296530. If you don't want to wait for that bug, please file a followup to move --enable-ctypes to js/moz.configure.
Attachment #8781247 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8781247 [details]
Bug 1294803 - Move MOZ_SYSTEM_FFI to moz.configure in preparation for moving libffi to our build system.

https://reviewboard.mozilla.org/r/71680/#review71312

> So, here we have a reason why it wouldn't work in js/moz.configure... until bug 1296530. If you don't want to wait for that bug, please file a followup to move --enable-ctypes to js/moz.configure.

Filed bug 1297471
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2938c8a38f2b
Move BUILD_CTYPES to Python configure. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/474dce563e74
Move MOZ_SYSTEM_FFI to moz.configure in preparation for moving libffi to our build system. r=glandium
https://hg.mozilla.org/mozilla-central/rev/2938c8a38f2b
https://hg.mozilla.org/mozilla-central/rev/474dce563e74
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Depends on: 1298740
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: