Closed Bug 1463637 Opened 2 years ago Closed 2 years ago

Update Hunspell build config to remove mozilla-config.h hacks

Categories

(Core :: Spelling checker, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: kmag, Assigned: kmag)

Details

Attachments

(1 file)

Our Hunspell build config currently uses mozilla-config.h.in to add some extra includes when building Hunspell sources. Adding hacks like that to the top-level build config really isn't ideal. We should add the extra includes via CFLAGS.
Comment on attachment 8979819 [details]
Bug 1463637: Remove mozilla-config.h hacks for Hunspell extra includes.

https://reviewboard.mozilla.org/r/245984/#review252024

Please remove the mentions to mozilla-config.h in those headers.

Please also r?build for a second round, in case they have something to say about the -include/-FI stuff. Not sure if a one-off is fine or if we should have a more general FORCE_INCLUDE.
Attachment #8979819 - Flags: review?(mh+mozilla)
Comment on attachment 8979819 [details]
Bug 1463637: Remove mozilla-config.h hacks for Hunspell extra includes.

https://reviewboard.mozilla.org/r/245984/#review252026

Let's not make that confusing for whoever takes this review in the build team by having a r+ from me, where the remaining r?build could be seen as leftover.
Attachment #8979819 - Flags: review?(mh+mozilla)
Priority: -- → P3
Attachment #8979819 - Flags: review?(core-build-config-reviews) → review?(ted)
Comment on attachment 8979819 [details]
Bug 1463637: Remove mozilla-config.h hacks for Hunspell extra includes.

https://reviewboard.mozilla.org/r/245984/#review252670

This seems fine.

::: extensions/spellcheck/hunspell/glue/common.mozbuild:8
(Diff revision 2)
> +# 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/.
> +
> +@template
> +def ForceInclude(*headers):

It might be worth moving this to build/templates.mozbuild, we do use `-FI` / `-include` in a few other places:
https://dxr.mozilla.org/mozilla-central/rev/47e81ea1ef10189ef210867934bf36e14cf223dc/media/gmp-clearkey/0.1/moz.build#60
https://dxr.mozilla.org/mozilla-central/rev/47e81ea1ef10189ef210867934bf36e14cf223dc/media/libav/moz.build#67
https://dxr.mozilla.org/mozilla-central/rev/47e81ea1ef10189ef210867934bf36e14cf223dc/media/libsoundtouch/src/moz.build#40
https://dxr.mozilla.org/mozilla-central/rev/47e81ea1ef10189ef210867934bf36e14cf223dc/security/moz.build#124
Attachment #8979819 - Flags: review?(ted) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
> > +@template
> > +def ForceInclude(*headers):
> 
> It might be worth moving this to build/templates.mozbuild, we do use `-FI` /
> `-include` in a few other places:
> https://dxr.mozilla.org/mozilla-central/rev/
> 47e81ea1ef10189ef210867934bf36e14cf223dc/media/gmp-clearkey/0.1/moz.build#60
> https://dxr.mozilla.org/mozilla-central/rev/
> 47e81ea1ef10189ef210867934bf36e14cf223dc/media/libav/moz.build#67
> https://dxr.mozilla.org/mozilla-central/rev/
> 47e81ea1ef10189ef210867934bf36e14cf223dc/media/libsoundtouch/src/moz.build#40
> https://dxr.mozilla.org/mozilla-central/rev/
> 47e81ea1ef10189ef210867934bf36e14cf223dc/security/moz.build#124

All of these happen in places where we're adding compiler-specific CFLAGs for other reasons anyway, so I'm not sure it would help much. But it probably wouldn't hurt either.
https://hg.mozilla.org/integration/mozilla-inbound/rev/79a77332a9e515bfae5e1807b5c57be7cec7c83a
Bug 1463637: Remove mozilla-config.h hacks for Hunspell extra includes. r=ted
https://hg.mozilla.org/mozilla-central/rev/79a77332a9e5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.