Closed Bug 1198226 Opened 5 years ago Closed 5 years ago

Move HOST_{C,CXX}FLAGS to moz.build

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: ted, Assigned: ted)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

What it says on the tin.
bug 1198226 - Add HOST_{C,CXX}FLAGS support to mozbuild frontend+recursive make backend. r?mshal
Attachment #8652304 - Flags: review?(mshal)
bug 1198226 - Add HOST_{C,CXX}FLAGS recursive make varible blacklist. r?mshal
Attachment #8652306 - Flags: review?(mshal)
Comment on attachment 8652305 [details]
MozReview Request: bug 1198226 - Move HOST_{C,CXX}FLAGS to moz.build HOST_{CFLAGS,CXXFLAGS,DEFINES}. r?mshal

https://reviewboard.mozilla.org/r/17119/#review15209

::: build/unix/stdc++compat/moz.build:26
(Diff revision 1)
> +    '-DMOZ_LIBSTDCXX_VERSION=$(MOZ_LIBSTDCXX_HOST_VERSION)',

Can we use '-DMOZ_LIBSTDCXX_VERSION=%s' % CONFIG['MOZ_LIBSTDCXX_HOST_VERSION'] or something instead of relying on a make backend to fill in the value?

::: modules/libmar/tool/moz.build:62
(Diff revision 1)
> +    '$(DEFINES)',

Does just DEFINES without the quotes/make substition work? Or maybe HOST_CFLAGS.extend(DEFINES). ie: have moz.build fill in the values here.

::: modules/libmar/tool/moz.build:66
(Diff revision 1)
> +    HOST_CFLAGS += ['-DMDCPUCFG=$(HOST_NSPR_MDCPUCFG)']

...=%s' % CONFIG['HOST_NSPR_MDCPUCFG']

::: modules/libmar/tool/moz.build:67
(Diff revision 1)
> +    CFLAGS += ['-DMDCPUCFG=$(HOST_NSPR_MDCPUCFG)']

Same as above.

::: other-licenses/bsdiff/Makefile.in:28
(Diff revision 1)
>  include $(topsrcdir)/config/rules.mk

You can delete this Makefile.in now, right?
Attachment #8652305 - Flags: review?(mshal)
Comment on attachment 8652304 [details]
MozReview Request: bug 1198226 - Add HOST_{CFLAGS,CXXFLAGS,DEFINES} support to mozbuild frontend+recursive make backend. r?mshal

https://reviewboard.mozilla.org/r/17117/#review15213
Attachment #8652304 - Flags: review?(mshal) → review+
Comment on attachment 8652306 [details]
MozReview Request: bug 1198226 - Add HOST_{C,CXX}FLAGS recursive make varible blacklist. r=mshal

https://reviewboard.mozilla.org/r/17121/#review15215
Attachment #8652306 - Flags: review?(mshal) → review+
Blocks: 1200360
https://reviewboard.mozilla.org/r/17119/#review15209

> Can we use '-DMOZ_LIBSTDCXX_VERSION=%s' % CONFIG['MOZ_LIBSTDCXX_HOST_VERSION'] or something instead of relying on a make backend to fill in the value?

Yeah, we can do that.

> Does just DEFINES without the quotes/make substition work? Or maybe HOST_CFLAGS.extend(DEFINES). ie: have moz.build fill in the values here.

Sadly, neither of those work. I'll file a followup on figuring out what this actually needs so we can make this less bad, but I'm going to leave this as-is for now.

> You can delete this Makefile.in now, right?

Yep, good catch!
Comment on attachment 8652304 [details]
MozReview Request: bug 1198226 - Add HOST_{CFLAGS,CXXFLAGS,DEFINES} support to mozbuild frontend+recursive make backend. r?mshal

bug 1198226 - Add HOST_{C,CXX}FLAGS support to mozbuild frontend+recursive make backend. r=mshal
Attachment #8652304 - Attachment description: MozReview Request: bug 1198226 - Add HOST_{C,CXX}FLAGS support to mozbuild frontend+recursive make backend. r?mshal → MozReview Request: bug 1198226 - Add HOST_{C,CXX}FLAGS support to mozbuild frontend+recursive make backend. r=mshal
Comment on attachment 8652304 [details]
MozReview Request: bug 1198226 - Add HOST_{CFLAGS,CXXFLAGS,DEFINES} support to mozbuild frontend+recursive make backend. r?mshal

bug 1198226 - Add HOST_{C,CXX}FLAGS support to mozbuild frontend+recursive make backend. r=mshal
Comment on attachment 8652305 [details]
MozReview Request: bug 1198226 - Move HOST_{C,CXX}FLAGS to moz.build HOST_{CFLAGS,CXXFLAGS,DEFINES}. r?mshal

bug 1198226 - Move HOST_{C,CXX}FLAGS to moz.build. r?mshal
Attachment #8652305 - Flags: review?(mh+mozilla)
Attachment #8652306 - Attachment description: MozReview Request: bug 1198226 - Add HOST_{C,CXX}FLAGS recursive make varible blacklist. r?mshal → MozReview Request: bug 1198226 - Add HOST_{C,CXX}FLAGS recursive make varible blacklist. r=mshal
Comment on attachment 8652306 [details]
MozReview Request: bug 1198226 - Add HOST_{C,CXX}FLAGS recursive make varible blacklist. r=mshal

bug 1198226 - Add HOST_{C,CXX}FLAGS recursive make varible blacklist. r=mshal
Comment on attachment 8652305 [details]
MozReview Request: bug 1198226 - Move HOST_{C,CXX}FLAGS to moz.build HOST_{CFLAGS,CXXFLAGS,DEFINES}. r?mshal

https://reviewboard.mozilla.org/r/17119/#review16253

As much as I don't really like multiplying moz.build variables, why not add HOST_DEFINES, which would make the whole thing with defines less clunky?

::: toolkit/crashreporter/google-breakpad/src/common/Makefile.in
(Diff revision 2)
> -ifneq (WINNT,$(OS_TARGET))
> -ifdef MOZ_CRASHREPORTER
> -endif
> -endif

Heh
Attachment #8652305 - Flags: review?(mh+mozilla) → review+
I thought about it, but what you see in this patch is the full extent of our usage of HOST_{C,CXX}FLAGS usage. Adding a HOST_DEFINES for these few places seemed kind of overkill. It still wouldn't solve bug 1200360 either, since DEFINES in moz.build wouldn't have the full set of DEFINES that wind up in the Makefile.
Well, HOST_DEFINES would at least have the advantage of consistency. (and, on the long run, I'd actually like to merge all the HOST_ things so that host programs/libs just use the DEFINES/CFLAGS/etc. variables, using HOST_DEFINES would make that more straightforward)
Comment on attachment 8652304 [details]
MozReview Request: bug 1198226 - Add HOST_{CFLAGS,CXXFLAGS,DEFINES} support to mozbuild frontend+recursive make backend. r?mshal

bug 1198226 - Add HOST_{CFLAGS,CXXFLAGS,DEFINES} support to mozbuild frontend+recursive make backend. r?mshal
Attachment #8652304 - Attachment description: MozReview Request: bug 1198226 - Add HOST_{C,CXX}FLAGS support to mozbuild frontend+recursive make backend. r=mshal → MozReview Request: bug 1198226 - Add HOST_{CFLAGS,CXXFLAGS,DEFINES} support to mozbuild frontend+recursive make backend. r?mshal
Comment on attachment 8652305 [details]
MozReview Request: bug 1198226 - Move HOST_{C,CXX}FLAGS to moz.build HOST_{CFLAGS,CXXFLAGS,DEFINES}. r?mshal

bug 1198226 - Move HOST_{C,CXX}FLAGS to moz.build HOST_{CFLAGS,CXXFLAGS,DEFINES}. r?mshal

As part of this move, HOST_NSPR_MDCPUCFG needed to be changed to get the quoting right.
Attachment #8652305 - Attachment description: MozReview Request: bug 1198226 - Move HOST_{C,CXX}FLAGS to moz.build. r?mshal → MozReview Request: bug 1198226 - Move HOST_{C,CXX}FLAGS to moz.build HOST_{CFLAGS,CXXFLAGS,DEFINES}. r?mshal
Attachment #8652305 - Flags: review?(mshal)
Comment on attachment 8652306 [details]
MozReview Request: bug 1198226 - Add HOST_{C,CXX}FLAGS recursive make varible blacklist. r=mshal

bug 1198226 - Add HOST_{C,CXX}FLAGS recursive make varible blacklist. r=mshal
Comment on attachment 8652304 [details]
MozReview Request: bug 1198226 - Add HOST_{CFLAGS,CXXFLAGS,DEFINES} support to mozbuild frontend+recursive make backend. r?mshal

Flagging for re-review: I added HOST_DEFINES here alongside the others, it fixes an issue I was having with that MDCPUCFG nonsense.
Attachment #8652304 - Flags: review+ → review?(mshal)
Attachment #8652304 - Flags: review?(mshal) → review+
Comment on attachment 8652304 [details]
MozReview Request: bug 1198226 - Add HOST_{CFLAGS,CXXFLAGS,DEFINES} support to mozbuild frontend+recursive make backend. r?mshal

https://reviewboard.mozilla.org/r/17117/#review16573
Attachment #8652305 - Flags: review?(mshal) → review+
Comment on attachment 8652305 [details]
MozReview Request: bug 1198226 - Move HOST_{C,CXX}FLAGS to moz.build HOST_{CFLAGS,CXXFLAGS,DEFINES}. r?mshal

https://reviewboard.mozilla.org/r/17119/#review16575

::: modules/libmar/tool/moz.build:71
(Diff revision 3)
> +    HOST_DEFINES['MDCPUCFG'] = CONFIG['HOST_NSPR_MDCPUCFG']

I think you are missing the regular DEFINES as well, right? The Makefile.in had:

ifdef CROSS_COMPILE
ifdef HOST_NSPR_MDCPUCFG
HOST_CFLAGS += -DMDCPUCFG=$(HOST_NSPR_MDCPUCFG)
CFLAGS += -DMDCPUCFG=$(HOST_NSPR_MDCPUCFG)
endif
endif

So you need:

DEFINES['MDCPUCFG'] = CONFIG['HOST_NSPR_MDCPUCFG'] inside the if statement as well, I think.
https://reviewboard.mozilla.org/r/17119/#review16575

> I think you are missing the regular DEFINES as well, right? The Makefile.in had:
> 
> ifdef CROSS_COMPILE
> ifdef HOST_NSPR_MDCPUCFG
> HOST_CFLAGS += -DMDCPUCFG=$(HOST_NSPR_MDCPUCFG)
> CFLAGS += -DMDCPUCFG=$(HOST_NSPR_MDCPUCFG)
> endif
> endif
> 
> So you need:
> 
> DEFINES['MDCPUCFG'] = CONFIG['HOST_NSPR_MDCPUCFG'] inside the if statement as well, I think.

I removed that on a hunch that it wasn't necessary because it seemed bogus. I was right. :)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.