928709 - Convert chromium-config.mk to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Brian O'Keefe [:bokeefe]]

I was going to try converting an entire directory tree of Makefile.ins to moz.build, but chromium-config.mk kept being the last thing in those Makefiles. For the moment, we can convert that to a chromium-config.mozbuild so it stops being a hindrance.

I'll also note that unlike bug 922566, chromium-config.mk adds an objdir path to LOCAL_INCLUDES for its ipdl headers, so -I$(TOPSRCDIR) doesn't help with that.

Try run at https://tbpl.mozilla.org/?tree=Try&rev=3f16d94018d7; patches in a moment.

Comment 1

[Brian O'Keefe [:bokeefe]]

Attachment: Add a moz.build token for LOCAL_INCLUDES in the objdir

Since LOCAL_INCLUDES are either relative to TOPSRCDIR or SRCDIR, I needed a way to add something relative to TOPOBJDIR/OBJDIR. This feels hacky, so I made it a separate variable GENERATED_INCLUDES. I'm assuming the only reason for including something from the objdir is that it's generated by the build system and not added to dist/<something>.

Comment 2

[Brian O'Keefe [:bokeefe]]

Attachment: Convert chromium-config.mk to chromium-config.mozbuild

Except for chromium-config.mk itself, I just moved the includes from Makefile.in to moz.build. I didn't try to clean up chromium-config.mk at all, except for un-nesting the 'else if' portions, so it's easier to read. I also stopped setting the OS_* variables, since they're not used anywhere, but I left them in comments on the if/elif/else lines.

Comment 3

[Michael Shal [:mshal]]

Comment on attachment 819444 [details] [diff] [review]
Add a moz.build token for LOCAL_INCLUDES in the objdir

This looks good for now. From looking through our current LOCAL_INCLUDES, it seems the GENERATED_INCLUDES are only necessary for ipc/ipdl headers (correct me if I'm wrong!) If we install them into dist/include (like for bug 883538), we might be able to pull this back out.

Comment 4

[Michael Shal [:mshal]]

Comment on attachment 819446 [details] [diff] [review]
Convert chromium-config.mk to chromium-config.mozbuild

>diff --git a/ipc/chromium/chromium-config.mozbuild b/ipc/chromium/chromium-config.mozbuild

>+if CONFIG['OS_ARCH'] == 'WINNT': #OS_WIN
>+    OS_LIBS += [ '$(call EXPAND_LIBNAME,psapi shell32 dbghelp)' ]

Bleh, these probably shouldn't be a passthrough. I'll add a note to clean this up later in the OS_LIBS bug so we can get rid of the make syntax.

>+else: #OS_POSIX

nit: What is the purpose of the #OS_FOO comments in all the if-statements? It looks like these correspond to variables that have since been removed from the Makefile.in/chromium-config.mk. If it isn't still providing anything useful today, we should probably just remove the comments. (Since it looks like the comment is just the same name that goes into DEFINES, I'd argue it's likely not useful).

Other than that, everything looks good to me! I don't think the reds in the try run are from this patch, but might be best to re-trigger them just to make sure :)

Comment 5

[Brian O'Keefe [:bokeefe]]

Attachment: Add a moz.build token for LOCAL_INCLUDES in the objdir, r=mshal, for checkin

(In reply to Michael Shal [:mshal] from comment #3)
> This looks good for now. From looking through our current LOCAL_INCLUDES, it
> seems the GENERATED_INCLUDES are only necessary for ipc/ipdl headers
> (correct me if I'm wrong!) If we install them into dist/include (like for
> bug 883538), we might be able to pull this back out.

It looks like most LOCAL_INCLUDES uses are sane, but there were a few places that include things from the objdir:
- b2g/app, browser/app, and webapprt/gtk2 include /build, presumably for application.ini.h
- A few subdirectories of js/src/ include the parent directory, which happens to be js/src
- media/webrtc/signaling/test includes dom/bindings
- A few places (chrome/src, and xpcom) include xpcom and xpcom/base

That said, I'm solidly in favor of doing those differently and pulling this out at some point.

(new patch for rebasing; carried forward r+)

Comment 6

[Brian O'Keefe [:bokeefe]]

Attachment: Convert chromium-config.mk to chromium-config.mozbuild, r=mshal, for checkin

(In reply to Michael Shal [:mshal] from comment #4)
> >+else: #OS_POSIX
> 
> nit: What is the purpose of the #OS_FOO comments in all the if-statements?
> It looks like these correspond to variables that have since been removed
> from the Makefile.in/chromium-config.mk. If it isn't still providing
> anything useful today, we should probably just remove the comments. (Since
> it looks like the comment is just the same name that goes into DEFINES, I'd
> argue it's likely not useful).

I had left them for posterity, I suppose. I pulled them out.

> Other than that, everything looks good to me! I don't think the reds in the
> try run are from this patch, but might be best to re-trigger them just to
> make sure :)

Since I had to rebase the other patch anyway, I went ahead and re-pushed: https://tbpl.mozilla.org/?tree=Try&rev=05d2fb1f00ca, which came back all green.

Comment 7

[Ed Morley [:emorley]]

Unfortunately this no longer applies cleanly to inbound - could you rebase?
(I'll try to land asap to avoid it bitrotting again)
Thank you :-)

Comment 8

[Brian O'Keefe [:bokeefe]]

Attachment: Convert chromium-config.mk to chromium-config.mozbuild, r=mshal, for checkin

(In reply to Ed Morley [:edmorley UTC+1] from comment #7)
> Unfortunately this no longer applies cleanly to inbound - could you rebase?
> (I'll try to land asap to avoid it bitrotting again)
> Thank you :-)

No problem. `hg rebase` didn't touch the other part of the patch, and managed this part automatically, so we should be good now.

Thanks :-)

Comment 9

[Ed Morley [:emorley]]

ah, I iirc rebase is more adept than qimport. Thank you :-)

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aa7cc951e821
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c031747aac8a

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aa7cc951e821
https://hg.mozilla.org/mozilla-central/rev/c031747aac8a