Move ctypes and libffi related variables to moz.configure

RESOLVED FIXED in Firefox 51

Status

()

Core
Build Config
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: chmanchester, Assigned: chmanchester)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

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

Attachments

(2 attachments)

(Assignee)

Description

a year ago
Namely, BUILD_CTYPES and MOZ_SYSTEM_FFI.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

a year ago
mozreview-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

::: 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 4

a year ago
mozreview-review
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 5

a year ago
mozreview-review
Comment on attachment 8781248 [details]
Bug 1294803 - Move BUILD_CTYPES to Python configure.

https://reviewboard.mozilla.org/r/71682/#review69270
Attachment #8781248 - Flags: review+
(Assignee)

Comment 6

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

a year ago
mozreview-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/#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+

Comment 10

a year ago
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

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c0e6aae0b93d
https://hg.mozilla.org/mozilla-central/rev/7f6047b2bc09
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 12

a year ago
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.
(Assignee)

Comment 13

a year ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2ee9681ce62f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/869dce10c19a
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
status-firefox51: fixed → affected

Comment 14

a year ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddc877cac511
needed a CLOBBER a=CLOBBER

Comment 15

a year ago
backoutbugherder
https://hg.mozilla.org/mozilla-central/rev/2ee9681ce62f
https://hg.mozilla.org/mozilla-central/rev/869dce10c19a
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Grr...
Status: RESOLVED → REOPENED
status-firefox51: fixed → affected
Resolution: FIXED → ---

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ddc877cac511
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
status-firefox51: fixed → affected
Resolution: FIXED → ---
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8781247 - Flags: review+ → review?(mh+mozilla)
(Assignee)

Updated

a year ago
Attachment #8781248 - Flags: review+ → review?(mh+mozilla)
(Assignee)

Comment 20

a year ago
These patches address comment 12 by putting BUILD_CTYPES in the root moz.configure.

Comment 21

a year ago
mozreview-review
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 22

a year ago
mozreview-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+
(Assignee)

Updated

a year ago
Blocks: 1297471
(Assignee)

Comment 23

a year ago
mozreview-review-reply
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

Comment 24

a year ago
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

Comment 25

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2938c8a38f2b
https://hg.mozilla.org/mozilla-central/rev/474dce563e74
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Depends on: 1298740
You need to log in before you can comment on or make changes to this bug.