Convert chromium-config.mk to moz.build

RESOLVED FIXED in mozilla27

Status

()

Core
Build Config
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bokeefe, Assigned: bokeefe)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 819444 [details] [diff] [review]
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>.
Attachment #819444 - Flags: review?(mshal)
(Assignee)

Comment 2

4 years ago
Created attachment 819446 [details] [diff] [review]
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.
Attachment #819446 - Flags: review?(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.
Attachment #819444 - Flags: review?(mshal) → review+
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 :)
Attachment #819446 - Flags: review?(mshal) → review+
(Assignee)

Comment 5

4 years ago
Created attachment 820965 [details] [diff] [review]
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+)
Attachment #819444 - Attachment is obsolete: true
Attachment #820965 - Flags: review+
(Assignee)

Comment 6

4 years ago
Created attachment 820969 [details] [diff] [review]
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.
Attachment #819446 - Attachment is obsolete: true
Attachment #820969 - Flags: review+
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
Keywords: checkin-needed

Comment 7

4 years ago
Unfortunately this no longer applies cleanly to inbound - could you rebase?
(I'll try to land asap to avoid it bitrotting again)
Thank you :-)
Keywords: checkin-needed
(Assignee)

Comment 8

4 years ago
Created attachment 821629 [details] [diff] [review]
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 :-)
Attachment #820969 - Attachment is obsolete: true
Attachment #821629 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed

Comment 9

4 years ago
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aa7cc951e821
https://hg.mozilla.org/mozilla-central/rev/c031747aac8a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.