Get rid of toolkit/content/Makefile.in

RESOLVED FIXED in Firefox 58

Status

RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: ted, Assigned: chmanchester)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla58
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment)

Right now toolkit/content/Makefile.in exists simply to set CXXFLAGS/CPPFLAGS in DEFINES for buildconfig.html:
https://hg.mozilla.org/mozilla-central/file/5e14887312d4523ab59c3f6c6c94a679cf42b496/toolkit/content/Makefile.in

I guess this is stuck here because the full value of $CXXFLAGS isn't computable without config.mk currently.
This is not super critical, in that it is just for about:buildconfig, but chmanchester: can we make this work more sensibly now with your CFLAGS work?
Blocks: 827343
(Assignee)

Comment 2

a year ago
Yep, after bug 1403346 we should be able to do this from moz.build.
Depends on: 1403346
(Assignee)

Updated

a year ago
Assignee: nobody → cmanchester
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
Comment on attachment 8926238 [details]
Bug 1256604 - Remove toolkit/content/Makefile.in and move setting CXXFLAGS for buildconfig.html to moz.build

https://reviewboard.mozilla.org/r/197494/#review202932

No flag change just to revisit this, or get someone who knows more to look at it.  Technically this looks fine.  But I don't understand a few things:
- Why are the original `CXXFLAGS` and `CPPFLAGS` not in the new set?
- We have some weird things happening in the tree, like http://searchfox.org/mozilla-central/source/browser/app/moz.build#118.  Does this global setting even make sense in the wild?

::: toolkit/content/moz.build:14
(Diff revision 1)
>  for var in ('target', 'MOZ_CONFIGURE_OPTIONS', 'CC', 'CC_VERSION', 'CXX'):
>      DEFINES[var] = CONFIG[var]
>  
>  DEFINES['CFLAGS'] = ' '.join(CONFIG['OS_CFLAGS'])
>  
> +cxx_flags = []

How do we make sure this set doesn't bit-rot as new places to stash flags are added?
(Assignee)

Comment 5

a year ago
mozreview-review-reply
Comment on attachment 8926238 [details]
Bug 1256604 - Remove toolkit/content/Makefile.in and move setting CXXFLAGS for buildconfig.html to moz.build

https://reviewboard.mozilla.org/r/197494/#review202932

The build backend hasn't actually set `CPPFLAGS` for a while, configure takes `CPPFLAGS` and exposes them as `OS_CPPFLAGS`. `CXXFLAGS` isn't used by the backend anymore (it was a little weird, it never contained all the flags, but imposed some mostly irellevant order between flags sets), the flags added in this patch reflect what used to be in `CXXFLAGS`.

> How do we make sure this set doesn't bit-rot as new places to stash flags are added?

We can either expose all the flags, potentially a larger set than was here originally, or make a case-by-case decision, and do our best not to add categories of flags, what we have already is probably more than necessary.

Comment 6

a year ago
mozreview-review
Comment on attachment 8926238 [details]
Bug 1256604 - Remove toolkit/content/Makefile.in and move setting CXXFLAGS for buildconfig.html to moz.build

https://reviewboard.mozilla.org/r/197494/#review203406

::: toolkit/content/buildconfig.html:53
(Diff revision 1)
>        <td>@CFLAGS@</td>
>      </tr>
>      <tr>
>        <td>@CXX@</td>
>        <td>@CC_VERSION@</td>
>  #ifndef BUILD_FASTER

You can remove this #ifndef (and fixup this comment:  https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/config/faster/rules.mk#63) since this now builds in the faster make backend.
Attachment #8926238 - Flags: review+
Attachment #8926238 - Flags: review?(core-build-config-reviews)
Comment hidden (mozreview-request)

Comment 8

a year ago
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7908ec3fc4af
Remove toolkit/content/Makefile.in and move setting CXXFLAGS for buildconfig.html to moz.build r=mshal

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7908ec3fc4af
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58

Updated

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