Closed Bug 1369433 Opened 3 years ago Closed 3 years ago

Variables in gyp_variables override values derived from gyp files

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: franziskus, Assigned: chmanchester)

References

Details

Attachments

(1 file)

We noticed that the Firefox config overrides gyp configurations on Android [1]. This breaks the build when gyp sets for example required cflags [2].

[1] https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/gyp_reader.py#329
[2] https://dxr.mozilla.org/mozilla-central/source/security/moz.build#117
Resummarizing. Amazingly this hasn't broken anything in the past, I guess it's just enough of an edge case? security/moz.build sets `CFLAGS` for Android in gyp_variables. franziskus has a patch that requires building an NSS source file with `-mpclmul` on  x86 with GCC. He added that to `cflags_mozilla` in the proper gyp file, but the `CFLAGS` from moz.build gyp_variables override it.
Summary: Firefox overrides gyp Android configuration → Variables in gyp_variables override values derived from gyp files
Comment on attachment 8873939 [details]
Bug 1369433 - Fix gyp_reader to take mozilla_cflags in to account when CFLAGS et al are set via sandbox_vars.

https://reviewboard.mozilla.org/r/145316/#review149340

Thanks, this should fix the issue at hand. Our gyp+moz.build integration is not incredibly well-factored in general. I'm not super inclined to worry about it because most Google projects are moving from gyp to gn at this point anyway.

::: python/mozbuild/mozbuild/frontend/gyp_reader.py:332
(Diff revision 1)
>          context['DISABLE_STL_WRAPPING'] = True
>  
> -        context.update(gyp_dir_attrs.sandbox_vars)
> +        for key, value in gyp_dir_attrs.sandbox_vars.items():
> +            if context.get(key) and isinstance(context[key], list):
> +                # If we have a key from sanbox_vars that's also been
> +                # populated here we use the value from sanbox_vars as our

typo: 'sandbox_vars'
Attachment #8873939 - Flags: review?(ted) → review+
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/96ec270d7778
Fix gyp_reader to take mozilla_cflags in to account when CFLAGS et al are set via sandbox_vars. r=ted
https://hg.mozilla.org/mozilla-central/rev/96ec270d7778
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Assignee: nobody → cmanchester
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 55 → mozilla55
You need to log in before you can comment on or make changes to this bug.